Skip to content

fix: model init, session cache, retry semantics; config accessors raise AttributeError for missing keys - #11

Open
wilsonhj wants to merge 1 commit into
americanexpress:mainfrom
wilsonhj:upstream/fix-model-session-retry
Open

fix: model init, session cache, retry semantics; config accessors raise AttributeError for missing keys#11
wilsonhj wants to merge 1 commit into
americanexpress:mainfrom
wilsonhj:upstream/fix-model-session-retry

Conversation

@wilsonhj

@wilsonhj wilsonhj commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

⚠️ Contains a deliberate accessor-contract change — please read first

This PR consolidates the model-initialization, session-cache, and retry-semantics fixes, and changes the config accessor contract, because the two are one atomic change set (verified empirically — see "Why config.py is in this PR" below):

  • Root Config.__getattr__ missing-key behavior: KeyErrorAttributeError
  • Nested ConfigWrapper.__getattr__ missing-key behavior: silent NoneAttributeError (breaking for downstream callers relying on the None return; item access cfg["key"] stays lenient)

The old silent-None made hasattr() always-True and getattr(x, k, default) never return its default — hiding config typos and forcing per-call-site workarounds throughout the codebase, which this PR removes.

Fixes

  • Silent exception swallowing in _get_direct_model_: a bare except: pass permanently discarded all model-init errors (wrong API key, missing provider package, network failure). Now: expected fallback conditions (ImportError/ValueError) log and fall through to manual init; unexpected errors re-raise as LCELModelConfigError with the original traceback chained. Error-message construction itself can no longer raise (safe getattr for model_name).
  • Provider handling: enumerated providers keep their manual init branches; non-enumerated providers are passed explicitly to init_chat_model(model_name, model_provider=...) so langchain-supported providers (mistral, groq, ollama, …) work, while genuinely unknown ones fail fast with a clear "not supported" error — and the check ordering no longer emits a misleading "falling back" warning or "API key not found" for a key that was never needed.
  • Azure routing: Azure-shaped configs route to a dedicated builder before the generic fast path (which has no notion of azure_endpoint/api_version/engine); endpoint detection parses the hostname and suffix-matches Azure domains (blocks lookalike-host/path spoofing); api_version is validated with a clear error on both direct and EAS paths.
  • SessionMap thread safety and correctness: is_expired() no longer raises KeyError for unregistered sessions; all reads/writes lock-guarded; singleton construction uses double-checked locking with full initialization before publication; get_valid_llm() does the existence+expiry check atomically; expires_in is captured per session (a later reconfiguration no longer retroactively changes cached sessions' expiry) and resolved before any network call.
  • Retry semantics: NonRetryableError marker family — permanent config errors fail fast instead of burning max_retry attempts, strictly per-family so a broad filter can't re-enable retries for unrelated marked exceptions; transient EAS auth failures (non-200) raise the retry-eligible EASAuthError instead of a permanent error.
  • Typing: wrap_llm_with_proxy accepts BaseLanguageModel (the real common ancestor of chat models) instead of the deprecated langchain.llms.BaseLLM import.

Why config.py is in this PR

The coupling is bidirectional: the rewritten consumers rely on missing-key AttributeError (root Config's old KeyError isn't swallowed by getattr(default)), and the new accessor breaks the old consumers' except KeyError paths. Splitting them produces two individually-red PRs; together the suite is fully green.

Verification

  • Full unit suite on this branch: 121 passed, 0 failed — including the chains/orchestrator/MCP tests still at main's state, confirming the accessor contract was the only cross-subsystem coupling.
  • Fixes main's own pre-existing test failure (test_model_with_unsupported_provider).
  • pylint on touched files: 9.89/10 vs 8.24/10 baseline; mypy: no new real errors (3 new import-not-found are guarded optional provider packages).

Sequencing

Includes the shared dependency pin (needed to build standalone); that hunk rebases away once #10 merges. Best reviewed/merged after the sanitizer/orchestrator PR.

Co-authored-by: Claude noreply@anthropic.com

…se AttributeError for missing keys

Config accessor contract change (BREAKING, and the reason this is one atomic
change set):
- Root Config attribute access for a missing top-level key now raises
  AttributeError instead of KeyError. KeyError escaped through hasattr() and
  getattr(obj, key, default), which only swallow AttributeError, so those
  optional-attribute idioms did not work on the root config object.
- Nested ConfigWrapper attribute access for a missing key now raises
  AttributeError instead of silently returning None. Silent-None made
  hasattr() always True and getattr(default) never return its default, so
  callers could not tell "unset" from "set" and misconfigurations surfaced as
  far-away crashes on unexpected Nones. This is BREAKING for downstream callers
  that relied on the None return; they must switch to getattr(obj, key,
  default) for legitimately optional keys. Item access (wrapper["key"] /
  wrapper[0]) intentionally stays lenient (None for missing), as the explicit
  "give me the value if present" accessor.

Why config.py ships together with its consumers here: the coupling is
bidirectional. model.py/token_util.py's new getattr-with-default and
`except AttributeError` config reads require the AttributeError contract (they
crash on the old KeyError), and the old model.py's `except KeyError` paths
would break under the new AttributeError contract. Splitting them would leave
whichever side landed first broken against the other, so the accessor change
and its consumers are a single atomic change set.

Consolidated fixes:
- model init error handling: silent exception swallowing in _get_direct_model_
  replaced with expected-vs-unexpected handling.
- explicit model_provider pass-through for non-enumerated providers with
  fail-fast LCELModelConfigError on unsupported/misconfigured providers.
- Azure endpoint detection by hostname suffix (anti-spoofing) with a dedicated
  direct-Azure builder and api_version validation.
- SessionMap: KeyError guard, per-session expires_in, double-checked-locking
  singleton, and atomic get_valid_llm.
- retry utilities: NonRetryableError family with per-family fail-fast.
- EASAuthError for transient auth failures.
- BaseLLM -> BaseLanguageModel proxy typing.

Notes:
- Includes a dependency pin (langchain/langchain-openai/langchain-community
  <0.4.0, langchain-mcp-adapters <0.2.0) that overlaps a separate deps PR and
  rebases away once that PR merges.
- Upstream main's own unit suite has one pre-existing failure
  (test_model_with_unsupported_provider) that this branch fixes.

Co-authored-by: Claude <noreply@anthropic.com>
@wilsonhj

wilsonhj commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Runtime verification — PASS ✅ (all 7 claims)

Verified through the public package boundary with the real Config.from_env() loader over hand-written YAMLs (CONFIG_PATH), no network calls, no test-suite rerun.

  1. Accessor contract: existing nested reads work; missing top-level and nested keys both raise AttributeError with the key named; getattr(cfg, 'nope', 'fallback') and hasattr now genuinely work; ConfigWrapper item access stays lenient (None).
  2. Unsupported provider fail-fast: provider: definitely-not-realLCELModelConfigError with the cause chained; captured logging output is empty — no misleading "falling back" warning.
  3. langchain-known but uninstalled provider (mistralai): clear error with the exact pip install -U langchain-mistralai instruction chained. (Cosmetic nit: the lead phrase "not supported" slightly undersells "just not installed" — the chained hint rescues it.)
  4. Azure anti-spoofing: https://notopenai.azure.com.evil.example/v1 does not route to the Azure builder (proceeds as plain OpenAI); a genuine *.openai.azure.com endpoint without api_version raises the clear Azure error before any opaque pydantic failure.
  5. SessionMap: is_expired() on an unregistered id returns True (no KeyError); round-trip via get_valid_llm(); singleton confirmed; staleness honored.
  6. Retry fail-fast by call counters: LCELModelConfigError (NonRetryable) → exactly 1 call; plain ValueError3 calls (max_retry); explicitly listing a marked family re-enables retry for that family only.
  7. Malformed config: missing models section → LCELModelConfigError: "No models defined in config" with the AttributeError cause chained.

Error-message quality — which is half of what this PR is about — held up under every adversarial probe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant