Python: Test actionable optional connector dependency errors - #14329
Python: Test actionable optional connector dependency errors#14329mikemikimike wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves Python connector optional-dependency failures by making the resulting ImportError messages actionable (naming the missing upstream package and the correct semantic-kernel[...] extra), and adds subprocess-isolated regression tests that remain valid even when extras are installed locally.
Changes:
- Add a parameterized subprocess test that blocks specific upstream packages via a
sys.meta_pathfinder and asserts the error message includes both the missing package and the corresponding install extra. - Improve the ONNX connector’s missing-dependency error message to reference
semantic-kernel[onnx]. - Add proactive Hugging Face import guarding and improve the Hugging Face prompt settings missing-dependency message to reference
semantic-kernel[hugging_face].
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| python/tests/unit/connectors/test_optional_dependencies.py | Adds subprocess-isolated regression coverage for actionable optional-dependency errors. |
| python/semantic_kernel/connectors/ai/onnx/services/onnx_gen_ai_completion_base.py | Updates the ONNX missing-dependency message to include the onnx extra install hint. |
| python/semantic_kernel/connectors/ai/hugging_face/services/hf_text_completion.py | Wraps Hugging Face imports to raise an actionable error referencing the hugging_face extra. |
| python/semantic_kernel/connectors/ai/hugging_face/hf_prompt_execution_settings.py | Makes the missing-transformers path raise an actionable error referencing the hugging_face extra. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| script = """ | ||
| import importlib | ||
| import importlib.abc | ||
| import sys | ||
|
|
||
| class MissingDependencyFinder(importlib.abc.MetaPathFinder): | ||
| def find_spec(self, fullname, path, target=None): | ||
| if fullname == sys.argv[3] or fullname.startswith(f"{sys.argv[3]}."): | ||
| raise ModuleNotFoundError(f"No module named '{sys.argv[3]}'", name=sys.argv[3]) | ||
| return None | ||
|
|
||
| sys.meta_path.insert(0, MissingDependencyFinder()) | ||
| try: | ||
| module = importlib.import_module(sys.argv[1]) | ||
| connector = getattr(module, sys.argv[2])() if sys.argv[5] else getattr(module, sys.argv[2])("unused") | ||
| if sys.argv[5]: | ||
| getattr(connector, sys.argv[5])() | ||
| except ImportError as exc: | ||
| print(exc) | ||
| else: | ||
| raise AssertionError("Expected an ImportError for the blocked optional dependency") | ||
| """ |
There was a problem hiding this comment.
Verified by running the subprocess test: the contents of the triple-quoted string start at column 0, so Python does not receive leading indentation. No code change is needed for this point.
| try: | ||
| imported = importlib.import_module("transformers") | ||
| ready = hasattr(imported, "GenerationConfig") | ||
| except ImportError: | ||
| ready = False |
There was a problem hiding this comment.
Fixed in a6a75a7 by catching only ModuleNotFoundError around the transformers probe. Other import-time ImportErrors are no longer converted into the optional-dependency fallback.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (1 commit(s)): 60e3b814d9c0
Model: claude-opus-4.8
Overview
This PR makes optional-dependency ImportError messages actionable by naming the pip extra (semantic-kernel[hugging_face] / semantic-kernel[onnx]) and adds a subprocess test that blocks upstream packages with a meta-path finder. The production changes are fail-closed and low-risk: the ONNX and Hugging Face guards raise before any degraded path executes, ModuleNotFoundError remains an ImportError subclass so existing catch sites are unaffected, and no APIs, dependency ranges, or successful-import behavior change. The one residual risk is in the new test itself: the Hugging Face parametrization asserts on a package name that is not guaranteed to surface (importing the submodule eagerly imports the completion module, whose import torch runs before transformers), so the case fails when torch is absent and never exercises the get_generation_config message it names — both reproduced during review.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
1 verified finding remained after source verification (1 medium) across 1 file. Details are attached to the affected lines below.
Affected areas: python/tests/unit/connectors/test_optional_dependencies.py
| text=True, | ||
| ) | ||
|
|
||
| assert package_name.replace("_", "-") in result.stdout |
There was a problem hiding this comment.
For the Hugging Face parametrization this assertion is environment-dependent and does not cover the path it names. Importing ...hugging_face.hf_prompt_execution_settings first executes the package __init__.py, which imports hf_text_completion, whose top-level import torch runs before from transformers import .... The meta-path finder blocks only transformers (argv[3]), so when torch is not installed the raised message is "torch is not installed..." and this assertion ("transformers" in stdout) fails — reproduced as 1 failed, 1 passed in an environment without the hugging_face extra. It passes in CI only because unit-all-except-dapr runs uv sync --all-extras, so torch is present and transformers surfaces first. A side effect is that the ImportError is raised at import time from hf_text_completion, so get_generation_config() (the method_name this row targets) is never invoked and its new message goes untested. Make the case hermetic: block both torch and transformers in the finder, or assert only on the extra token (f"semantic-kernel[{extra_name}]", already checked on the next line) rather than a specific package name that is not guaranteed to surface first.
There was a problem hiding this comment.
Fixed in 984c0d3. The Hugging Face case now loads hf_prompt_execution_settings.py directly, bypassing the package init imports, blocks transformers, and invokes get_generation_config(). I reproduced the old environment-dependent behavior by forcing torch to be missing (the old assertion failed with the torch message), then verified the updated focused test passes for both connectors.
Summary
hugging_faceSemantic Kernel extraonnxextraFixes #14328
Implementation and compatibility
The test covers the existing optional-dependency paths for
transformersandonnxruntime_genai. Hugging Face now catches import failures before its service module leaks a genericModuleNotFoundError; ONNX keeps its existing lazy import behavior and only improves the message. No dependency ranges, public APIs, successful-import behavior, provider calls, or credentials are changed.Validation
Passed:
python -m pytest tests/unit/connectors/test_optional_dependencies.py tests/unit/connectors/ai/onnx/test_onnx_prompt_execution_settings.py -q— 9 passedpython -m ruff check semantic_kernel/connectors/ai/hugging_face/__init__.py semantic_kernel/connectors/ai/hugging_face/hf_prompt_execution_settings.py semantic_kernel/connectors/ai/hugging_face/services/hf_text_completion.py tests/unit/connectors/test_optional_dependencies.pypython -m ruff check --ignore RUF070 semantic_kernel/connectors/ai/onnx/services/onnx_gen_ai_completion_base.pypython -m ruff format --checkon all four changed filespython -m compileall -qon all four changed filesgit diff --check upstream/main...HEADNot fully run / environment limitations:
onnxruntime_genaioptional package; without it, 21 tests fail at their existing mock/import setup and 10 pass.semantic_kernel/connectors/mcp.pywith the locally installed MCP version;--follow-imports=skipstill reports three pre-existing override errors inhf_text_completion.py.Evidence self-check (round 1)
transformers is not installed.andonnxruntime-genai is not installed.without install extras. The revised subprocess test also exposed the Hugging Face package-level genericModuleNotFoundErrorpath.semantic-kernel[hugging_face]/semantic-kernel[onnx]. Both cases pass after the patch.Evidence self-check (round 2)
a6a75a766a3775d909c16148f5363d99e96ef33a,python -m pytest tests/unit/connectors/test_optional_dependencies.py tests/unit/connectors/ai/onnx/test_onnx_prompt_execution_settings.py -qreports 9 passed. PR files show only the two representative connector paths, their import guard, and the parameterized regression test.ImportErrortoModuleNotFoundError, so unrelated import-time failures are not rewritten. Public APIs, dependency ranges, successful imports, provider calls, data, security, and performance behavior remain unchanged.sys.meta_pathand module cache state cannot leak between cases; process exit owns cleanup.git diff --check. Local HEAD, fork ref, and PR head all matcha6a75a766a3775d909c16148f5363d99e96ef33a.