fix: model init, session cache, retry semantics; config accessors raise AttributeError for missing keys - #11
Open
wilsonhj wants to merge 1 commit into
Conversation
…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>
This was referenced Jul 8, 2026
Merged
Contributor
Author
Runtime verification — PASS ✅ (all 7 claims)Verified through the public package boundary with the real
Error-message quality — which is half of what this PR is about — held up under every adversarial probe. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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):
Config.__getattr__missing-key behavior:KeyError→AttributeErrorConfigWrapper.__getattr__missing-key behavior: silentNone→AttributeError(breaking for downstream callers relying on theNonereturn; item accesscfg["key"]stays lenient)The old silent-
Nonemadehasattr()always-True andgetattr(x, k, default)never return its default — hiding config typos and forcing per-call-site workarounds throughout the codebase, which this PR removes.Fixes
_get_direct_model_: a bareexcept: passpermanently 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 asLCELModelConfigErrorwith the original traceback chained. Error-message construction itself can no longer raise (safegetattrformodel_name).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_endpoint/api_version/engine); endpoint detection parses the hostname and suffix-matches Azure domains (blocks lookalike-host/path spoofing);api_versionis validated with a clear error on both direct and EAS paths.SessionMapthread safety and correctness:is_expired()no longer raisesKeyErrorfor 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_inis captured per session (a later reconfiguration no longer retroactively changes cached sessions' expiry) and resolved before any network call.NonRetryableErrormarker family — permanent config errors fail fast instead of burningmax_retryattempts, 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-eligibleEASAuthErrorinstead of a permanent error.wrap_llm_with_proxyacceptsBaseLanguageModel(the real common ancestor of chat models) instead of the deprecatedlangchain.llms.BaseLLMimport.Why config.py is in this PR
The coupling is bidirectional: the rewritten consumers rely on missing-key
AttributeError(rootConfig's oldKeyErrorisn't swallowed bygetattr(default)), and the new accessor breaks the old consumers'except KeyErrorpaths. Splitting them produces two individually-red PRs; together the suite is fully green.Verification
main's state, confirming the accessor contract was the only cross-subsystem coupling.main's own pre-existing test failure (test_model_with_unsupported_provider).import-not-foundare 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