fix(web): a benched lazy-install must not disable an importable SDK - #80022
fix(web): a benched lazy-install must not disable an importable SDK#80022rodrigogs wants to merge 1 commit into
Conversation
3a8c89e to
37f6173
Compare
26124b0 to
31521a2
Compare
|
Hi — heads-up that CI has never actually run on this PR: every workflow run (CI + Docker Build) since it was opened ends in |
81f62b9 to
a47b18a
Compare
|
Rebased onto current Conflict-free, and nothing upstream disturbed this PR — including the defect itself: Verification on the new head: One deliberate non-change, kept so the diff stays identical to what was reviewed: This repository does not run CI on pull requests from forks, so the checks tab stays empty and |
`_ensure_parallel_sdk_installed` promised, in its own docstring, to swallow a benign availability error and let `from parallel import ...` be the real gate. The handlers did the opposite: except ImportError: pass # never fired except Exception: raise ImportError() # fired for the intended case `lazy_deps.ensure` signals an unusable feature with `FeatureUnavailable`, which subclasses RuntimeError, not ImportError. So the narrow arm never matched, the broad arm turned "cannot install" into a hard failure, and the import that was supposed to decide was never reached. On any host with `security.allow_lazy_installs=false` the Parallel provider was unusable whether or not `parallel-web` was actually present. Catch FeatureUnavailable explicitly and only fail when the package is genuinely absent, keeping the actionable install hint. `_parallel_sdk_importable` checks `sys.modules` before `find_spec`, because an already-imported module is importable by definition and `find_spec` raises ValueError on entries with no `__spec__` — which is how the existing tests inject a stub SDK. Unrelated faults still surface as ImportError for the caller. This is why tests/tools/test_web_tools_config.py::TestParallelClientConfig failed on such a host: the suite installs a stub `parallel` module in `sys.modules`, but production raised before ever looking at it.
a47b18a to
7671a6a
Compare
|
Force-pushed a metadata-only fix so the contributor attribution check can pass.
Every commit's author is now |
Problem
_ensure_parallel_sdk_installedinplugins/web/parallel/provider.pydocumented one contract and implemented the opposite. Its docstring promised to swallow a benign availability error from the lazy-deps helper and let the subsequentfrom parallel import ...be the real gate on whether the SDK is usable. The handlers were:The root cause is a wrong exception class.
tools.lazy_deps.ensuresignals an unusable feature by raisingFeatureUnavailable, which subclassesRuntimeError— notImportError. So:except ImportErrorarm never matched the error it was written for;except Exceptionarm caughtFeatureUnavailableand converted "cannot install right now" into a hardImportError;The practical consequence: on any host with
security.allow_lazy_installs=false, the Parallel provider was dead whether or notparallel-webwas actually installed. Declining to install a package was treated as the package being absent. A correctly provisioned host that hadparallel-webpresent but lazy installs disabled by policy got the same failure as a host missing the dependency entirely.Fix
Try the install; if it is impossible, then check whether it was needed at all. Only a genuinely missing package raises, and the raised error keeps the actionable install hint:
Unrelated faults (an
OSErrorfrom the installer, say) are real problems and still surface to the caller asImportErrorrather than being silently swallowed. AnImportErrorfrom importing the lazy-deps helper itself returns early and lets the import decide, which is what the original docstring described.Why
sys.modulesis checked beforefind_specTwo reasons, and the order matters:
sys.modulesfirst is both correct and cheaper than a filesystem scan.importlib.util.find_specraisesValueErroron a module present insys.moduleswhose__spec__isNone. That is not a hypothetical: it is exactly how the existing test suite injects a stub SDK — a baretypes.ModuleType("parallel")has no__spec__. Callingfind_specfirst would raiseValueErroron precisely the inputs the check needs to answerTruefor.ValueErroris caught as well, so a malformed entry degrades to "not importable" instead of escaping as an unexpected exception type.This also explains the pre-existing failure in
tests/tools/test_web_tools_config.py::TestParallelClientConfig: the suite installs a stubparallelmodule insys.modules, but production code raised before ever looking at it.Test evidence
tests/tools/test_parallel_sdk_ensure.py(new) covers the three distinct paths, since the bug was that one exception class was being conflated with another:test_importable_sdk_survives_disabled_lazy_installs—FeatureUnavailableraised while the package imports fine: not an error.test_missing_sdk_still_reports_the_install_hint— genuinely absent package still raisesImportErrorcarryingparallel-web.test_unrelated_failure_is_still_surfaced— anOSErrorfrom the installer is not swallowed.Reverting only
provider.pyto its currentmaincontent, with the new tests in place, fails as expected — confirming the production change is what the tests are pinning rather than the tests passing incidentally:Note that the two
TestParallelClientConfigfailures are pre-existing onmainand are fixed by this change; they were the symptom that led to the bug.Context
Found alongside #79839 and #79840 while investigating a single incident; the three are independent fixes in different subsystems and can be reviewed and merged in any order. This branch is based on current
main(ff3793fdf) and touches two files:plugins/web/parallel/provider.pyand the new test module.