Repository navigation
[BREAKING] Python: Honor injected Azure AI Search clients over settings - #10001
lsmlhi_25 (FOWEPJF255) wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues were identified for the injected-client precedence and ownership changes.
No open issues 🎉
What changed in this PR
This PR addresses #9956 by making injected Azure AI Search clients take precedence over connection settings and remain caller-owned. This intentionally breaks cleanup behavior for callers that previously used managed_client=True to transfer ownership.
Changes:
- Accept connection and settings-file arguments alongside injected clients without resolving those settings.
- Preserve cleanup of connector-created clients, including search clients derived from an injected index client.
- Update regression tests and documentation to reflect the precedence and ownership rules.
| File | Description |
|---|---|
python/packages/azure-ai-search/tests/test_vector_store.py |
Updates ownership assertions and tests injected-client precedence over explicit settings. |
python/packages/azure-ai-search/README.md |
Documents caller ownership even with managed_client=True. |
python/packages/azure-ai-search/AGENTS.md |
Aligns connector guidance with settings bypass and ownership behavior. |
python/packages/azure-ai-search/agent_framework_azure_ai_search/_vector_store.py |
Removes settings conflicts and limits cleanup ownership to connector-created clients. |
Review effort: Balanced
|
The merge gate identified type narrowing errors in the new precedence test. I split the store and collection assertions into their respective branches and pushed dd1e9fd. The 20 targeted cases, Ruff, and all five azure-ai-search test typing checkers pass locally; CI is rerunning. |
Muhammad Abdullah Butt (1AbdullahButt)
left a comment
There was a problem hiding this comment.
The precedence and ownership changes look right. There's one gap against the plan on #9956: managed_client=True is now ignored silently.
What I checked at this head (_vector_store.py).
- Collection.
_create_index_clientruns only when neither client is injected (:332-339), so settings and.envare never read when a client is supplied. An injected client is never added to_owned_clients. ASearchClientderived from an injectedindex_clientis still added (:341-345), so the collection closes the client it created and leaves the borrowed index client open. That's the case the description asks reviewers to look at, and it's right. Asearch_client-only collection owns nothing, somanaged_clientisFalse. - Store.
owned = index_client is None(:935), andclose()only closesself.index_clientwhenmanaged_clientis set (:1045-1046). Collections fromget_collectionstill close their derived search clients through theAsyncExitStack, so one failingclose()doesn't skip the others. - Checks that still apply. The
query_source_credentialTypeErrorstill runs first in both constructors. The preview,index_optionsand auto-key checks don't depend on client creation and are unchanged. - Tests. The tests flipped from "rejects" to "ignores" cover all four kinds and five overrides. With
managed_client=True, they assert thatSearchIndexClient(the factory) is never called and that injected clients are never awaited-closed. git merge-treeagainstmainis clean. The branch is about 165 commits behind, and the only one of those that touches this package is the 1.25.0 release bump (#10151).
The gap: managed_client=True is ignored with no signal. In the /plan comment on #9956, the maintainer asked to "resolve the public managed_client compatibility contract ... (retain-and-ignore, deprecate, or remove)". The bot plan there recommends retain-and-ignore with a DeprecationWarning, so that callers who relied on framework cleanup aren't left leaking connections without knowing. This PR keeps the parameter and drops its effect. A small helper would close the gap:
if managed_client and (index_client is not None or search_client is not None):
warnings.warn(
"managed_client=True no longer transfers ownership of injected clients; "
"close them yourself or pass connection options instead.",
DeprecationWarning,
stacklevel=2,
)Call it from both constructors, and add a pytest.warns(DeprecationWarning) case. managed_client=False/None should stay silent.
For the release notes: recent [BREAKING] entries in python/CHANGELOG.md include a **Migration:** paragraph. A two-line migration note in the PR description ("close injected clients yourself, e.g. async with client:") would make the 1.27 entry easy to write.
I didn't run the tests.
Reviewed with help from Claude Code.
|
Thanks for the detailed review. Addressed in bbb5752: both constructors now emit a I added regression cases for the warning and silent paths, clarified the package README and constructor docstrings, and added a migration note to the PR description. The Azure AI Search package's non-integration tests pass locally, as do Ruff check, Ruff format, and the staged diff check. CI is running on the new head. |
Motivation & Context
An application that injects a working Azure AI Search SDK client cannot currently pass an endpoint, credential, API key, or settings-file option to the store or collection constructor: construction raises before the supplied client is used. Passing
managed_client=Truecan also cause the connector to close that caller-owned client. This blocks dependency-injected retrieval setups that carry ambient connection configuration.Description & Review Guide
What are the major changes? Supplied clients now take precedence over client-creation settings. Store and collection cleanup always leaves injected clients open, while clients created by the connector are still closed. Focused regression tests cover explicit overrides, settings bypass, both client lifecycles, and the deprecation warning; the connector documentation reflects the behavior.
What is the impact of these changes? Previously rejected combinations now construct successfully.
managed_client=Trueremains accepted for compatibility but no longer transfers ownership of an injected client. It now emits aDeprecationWarningso callers relying on framework cleanup can migrate.Migration: Close injected clients yourself, for example with
async with client:. Pass connection options instead when the connector should own and close the client.What do you want reviewers to focus on? Please check that the connector still closes a search client it creates from an injected index client, without closing the injected index client.
Related Issue
Fixes #9956
AI Assistance
AI assistance details: Issue analysis, implementation, regression tests, connector documentation, and this description were materially assisted by an AI coding assistant. The change was reviewed against the source and verified with the package's non-integration tests and Ruff.
Contribution Checklist
[BREAKING]for review.