Skip to content

[BREAKING] Python: Honor injected Azure AI Search clients over settings - #10001

Open
lsmlhi_25 (FOWEPJF255) wants to merge 3 commits into
microsoft:mainfrom
FOWEPJF255:FOWEPJF255/issue-9956-azure-search-injected-client
Open

lsmlhi_25 (FOWEPJF255) wants to merge 3 commits into
microsoft:mainfrom
FOWEPJF255:FOWEPJF255/issue-9956-azure-search-injected-client

Conversation

@FOWEPJF255

@FOWEPJF255 lsmlhi_25 (FOWEPJF255) commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

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=True can 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=True remains accepted for compatibility but no longer transfers ownership of an injected client. It now emits a DeprecationWarning so 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

  • No material AI assistance was used.
  • This is an AI-assisted contribution. I reviewed, understood, and verified all submitted content and accept responsibility for it.

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

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • This PR links to an agreed issue with no competing open PR, or the Related Issue section documents a trivial-change or repository-automation exception.
  • This is not a breaking change. This PR intentionally changes the experimental connector's ownership behavior; the title carries [BREAKING] for review.

Copilot AI balanced review requested due to automatic review settings November 3, 2026 01:47
@agent-framework-automation agent-framework-automation Bot added documentation Usage: [Issues, PRs], Target: documentation in the code base and learn docs python Usage: [Issues, PRs], Target: Python labels Nov 3, 2026
@agent-framework-automation agent-framework-automation Bot added the breaking change Usage: [PRs], Target: all PRs that introduce changes that are not backward compatible label Nov 3, 2026
@FOWEPJF255
lsmlhi_25 (FOWEPJF255) marked this pull request as ready for review November 3, 2026 01:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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

@FOWEPJF255

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_client runs only when neither client is injected (:332-339), so settings and .env are never read when a client is supplied. An injected client is never added to _owned_clients. A SearchClient derived from an injected index_client is 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. A search_client-only collection owns nothing, so managed_client is False.
  • Store. owned = index_client is None (:935), and close() only closes self.index_client when managed_client is set (:1045-1046). Collections from get_collection still close their derived search clients through the AsyncExitStack, so one failing close() doesn't skip the others.
  • Checks that still apply. The query_source_credential TypeError still runs first in both constructors. The preview, index_options and 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 that SearchIndexClient (the factory) is never called and that injected clients are never awaited-closed.
  • git merge-tree against main is 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.

@FOWEPJF255

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review. Addressed in bbb5752: both constructors now emit a DeprecationWarning when managed_client=True is paired with an injected client. False, None, and connector-created clients remain silent. The injected client remains caller-owned.

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.

This branch was successfully deployed

1 active deployment
github-app-auth — bbb57527 Deployed Nov 14, 2026 by FOWEPJF255 via add_label #27849
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Usage: [PRs], Target: all PRs that introduce changes that are not backward compatible documentation Usage: [Issues, PRs], Target: documentation in the code base and learn docs python Usage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python: [Bug]: Azure AI Search injected clients do not take precedence over connection settings

3 participants