Repository navigation
perf(programs): register optional SDK program types lazily - #1499
TheGupta2012 wants to merge 14 commits into
Conversation
Importing qbraid.programs used to import every installed quantum SDK (cirq, qiskit, qrisp, bloqade, ...) just to build the program type registry. With qrisp installed this alone took ~3.6 s. - Replace the eager _dynamic_importer with LazyTypeRegistry. Aliases are listed when the package is found on disk (no import); the type is imported on first lookup. - Type checks in get_program_type_alias and register_program_type only look at entries whose module is already imported, which is exact: a program can only be an instance of a type whose module is loaded. - QPROGRAM_TYPES and QPROGRAM_NATIVE are built on first read. - gate_model, analog and annealing import their SDK wrapper submodules on attribute access instead of at package import. - A broken SDK install is now dropped with a warning instead of silently. - The "no matching type" error lists aliases instead of types, so get_program_type_alias(..., safe=True) no longer imports every SDK. - Add tests for the lazy registry and a guard that importing and using qbraid.programs on non-SDK input imports no optional SDK. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MnewW15ZyxcusY61HhPBCU
Argus reviewAuto-review is off for this repo. Tick the box below to run a review on this PR. Running Argus review... Estimated cost
Tip: you can also comment |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
 |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MnewW15ZyxcusY61HhPBCU
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This comment has been minimized.
This comment has been minimized.
![argus-eye[bot]](https://avatars.githubusercontent.com/in/2969822?s=60&v=4){kind=small}
There was a problem hiding this comment.
Argus · 10/10 — No findings
Argus reviewed 11 files and checked 10 for bugs, security, architecture, and regressions. Custom agent ponytail-review also ran. The diff matches the stated goal.
What this PR does · 10 author checks
- Make the program type registry lazy so installed SDKs are listed immediately but imported only when their program types are actually used.
- Excludes: Making conversion packages lazy; ConversionGraph() still imports SDKs through conversion modules and is planned for a follow-up.
- Excludes: Reducing the remaining roughly 0.4 s base import cost from typer.py and experiment.py; part of the fix belongs in qbraid-core.
Author's checks
- LazyTypeRegistry replaces _dynamic_importer and maps aliases to module and class names.
- SDK presence checks do not import SDKs or parent packages; dotted paths are searched using PathFinder.
- Program classes are imported on first lookup; lookup failures log the module and error before dropping the alias.
- get_program_type_alias checks only entries whose modules are already imported, and no-match errors list aliases instead of types.
- QPROGRAM_NATIVE is built when read; register_program_type and unregister_program_type scan only loaded entries.
- gate_model, analog and annealing wrapper submodules load on attribute access through PEP 562 getattr.
- tests/programs/test_lazy_registry.py covers the registry and guards against optional SDK imports during common calls.
- Aliases, native aliases and resolved type names remain identical to main across per-extra and full environments.
- The full test suite passes on Python 3.12 with requirements-dev.txt installed.
- Black, isort, ruff and pylint pass with the versions pinned in tox.ini.
3 findings suppressed by team feedback (audit)
usage: 985.4k tokens · 48 stages
Contract: production/full · checked: bug_hunter, security, architecture, regression, single-pass review, ponytail-review · 3 suppressed by team feedback · review took 3m27s
| Stage | Model | Tokens | Cost |
|---|---|---|---|
| Intent | openai/gpt-6.1-sol |
6.4k | $0.0131 |
| Triage | openai/gpt-6.1-sol |
16.4k | $0.0162 |
| Lead agent | openai/gpt-6.1-sol |
3.5k | $0.0186 |
| Review specialists (43) | openai/gpt-6.1-sol |
928.6k | $2.0032 |
| Review | openai/gpt-6.1-sol |
27.1k | $0.0545 |
| Scoring | openai/gpt-6.1-sol |
3.4k | $0.0049 |
Dashboard → · @argus-eye help
🧠 Learned: 1 pattern · 1 PR summary · 1 architecture note
Cover a module without a spec, a missing attribute on an imported module, items() skipping broken entries, LazyTypeSet.copy/update and unregistering a never-loaded alias. Drop a redundant re-check inside the resolve lock; the lock already serializes resolution. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MnewW15ZyxcusY61HhPBCU
ryanhill1
left a comment
ryanhill1
left a comment
There was a problem hiding this comment.
Thanks for this; the import-time gains are substantial. Requesting changes for one blocking regression and a few smaller compatibility breaks, all commented inline.
The blocking one: SDKs are now registered when find_spec finds them, without importing them, so an installed SDK that fails to import gets registered. The conversion graph then imports it and catches only ModuleNotFoundError, so one broken optional install makes every transpile() fail. main skips that SDK and keeps working.
Address review on #1499. An SDK found on disk but failing to import is now listed in the registry, so callers must not assume "listed" means "importable": - conversions: skip (and log) an SDK whose conversion module raises on import instead of failing every ConversionGraph()/transpile() call. - gate_model/analog/annealing: a wrapper whose SDK fails to import raises AttributeError and is dropped from submodules/__all__, so hasattr() and star-imports keep working. - IonQ device and Conversion error path use QPROGRAM_REGISTRY.get(). - LazyTypeRegistry supports copy/deepcopy/pickle and | / |= like dict. - LazyTypeSet set operators and copies return plain sets. - Changelog notes the registries are mappings, not dict subclasses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MnewW15ZyxcusY61HhPBCU
Resolving an alias whose SDK fails to import drops it from QPROGRAM_ALIASES, which made ConversionGraph(require_native=True) raise 'Set changed size during iteration'. Iterate a snapshot instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MnewW15ZyxcusY61HhPBCU
The broken-SDK regression test runs in a subprocess, which coverage does not measure. Exercise the conversion-skip branch and the subpackage AttributeError branch in process as well. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MnewW15ZyxcusY61HhPBCU
ryanhill1
left a comment
ryanhill1
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. 4fceb58 and ec50b71 fix the broken-SDK crash and the alias-set mutation. A few things are still open, and I've replied in the existing threads where they continue one:
conversions/__init__.py:107: the new broadexceptalso hides bugs in qbraid's own conversion modules (reply on the_availablethread).QPROGRAM_TYPESstill has no named set methods (reply on theLazyTypeSetthread).from qbraid.programs.gate_model import *still fails on a broken SDK, and the test passes only because of the order of its asserts (reply on thesubmodulesthread).IonQDevice.runnow imports qiskit on every call (inline below).
- conversions: re-raise unless the SDK itself fails to import, so bugs in qbraid's own conversion modules (including qasm2/qasm3) still surface. - IonQ device: only look up the qiskit type when qiskit is already imported; a program cannot be a qiskit circuit otherwise. - LazyTypeSet: add union, intersection, difference, symmetric_difference, issubset and issuperset. - gate_model/analog/annealing: keep SDK wrappers out of __all__ and list them in __dir__, so a star-import neither imports every SDK nor fails on a broken one. - Tests: run the star-import before hasattr in the broken-SDK test, split the conversion test into skip-broken-SDK and surface-own-bug cases, and cover the named set methods. - Changelog: note QPROGRAM_TYPES is a MutableSet and the narrower star-import. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MnewW15ZyxcusY61HhPBCU
…patia-6aiin0 # Conflicts: # CHANGELOG.md
…oken The guard re-raised whenever lib's own SDK loaded, but some conversion modules import another SDK at the top: pyquil's import cirq. With pyquil working and cirq installed but broken, the ImportError escaped and import qbraid.transpiler failed. An ImportError now skips that lib's conversions with the warning, unless the module that failed to import is qbraid's own, which still raises along with any other error.
… is raised The guard classified failures by ImportError.name, which was wrong both ways. A qbraid conversion importing a name a newer cirq removed raises ImportError with name 'cirq.ops', so it was skipped with a warning instead of raised. And a broken second SDK usually fails with AttributeError or TypeError, which still crashed import qbraid.transpiler. A failure is now a qbraid bug when its innermost frame is inside the qbraid package; anything raised inside an SDK is a broken install and is skipped with the warning. ModuleNotFoundError goes through the same check, so a missing qbraid module raises instead of being swallowed, while an optional SDK that isn't installed is still skipped quietly. The traceback is walked by hand because traceback.extract_tb obeys sys.tracebacklimit, which is 0 in some environments.
|
@Argus-Eye review |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Argus · 9/10 — Broken wrapper imports disappear without warning
Check 2 warnings before you merge. Argus also found 1 suggestion in 15 files.
- 🟡 Warning · Broken wrapper imports disappear without warning ·
qbraid/programs/gate_model/__init__.py:L74 - 🟡 Warning · Missing SDK submodules bypass error reporting ·
qbraid/transpiler/conversions/__init__.py:L130 - 💡 Suggestion · Mismatch diagnostics still resolve optional SDK types ·
qbraid/transpiler/edge.py:L243
What this PR does · 12 author checks
- Register optional SDK program types lazily so installed SDKs are imported only when their types are used, avoiding import costs for unused SDKs.
- Excludes: Eliminate SDK imports caused by top-level imports in ConversionGraph conversion modules.
- Excludes: Reduce the remaining base import cost from pyqasm, openqasm3 and the qbraid-core client.
Author's checks
- LazyTypeRegistry lists aliases when packages exist without importing them and imports each class on first lookup.
- Type detection checks only already-imported SDKs and preserves the detected alias; no-match errors list alias names.
- QPROGRAM_NATIVE is built when read, and registration and unregistration scan only loaded entries.
- gate_model, analog and annealing wrapper modules load on attribute access.
- Broken SDK installs log a warning rather than vanishing silently.
- A regression test fails if common calls import an optional SDK.
- The full suite passes with every SDK in requirements-dev.txt installed.
- Aliases and resolved types match main in all 23 verification virtual environments.
- Black, isort, ruff and pylint pass with pinned versions.
- ConversionGraph iterates a snapshot of QPROGRAM_ALIASES to avoid set-size changes when resolving broken SDK aliases.
- Conversion imports skip and warn for broken SDKs, while failures originating in qbraid's own conversion code still surface.
- IonQ device looks up the qiskit type only when qiskit is already imported.
3 findings suppressed by team feedback (audit)
⚠️ Argus could not finish 1 file check, so this review is partial. More findings can exist.
usage: 1.5M tokens · 64 stages
Contract: production/full · checked: bug_hunter, security, architecture, regression, single-pass review, ponytail-review · 3 suppressed by team feedback · review took 3m46s
| Stage | Model | Tokens | Cost |
|---|---|---|---|
| Intent | openai/gpt-6.1-sol |
7.7k | $0.0141 |
| Triage | openai/gpt-6.1-sol |
22.3k | $0.0215 |
| Lead agent | openai/gpt-6.1-sol |
4.7k | $0.0246 |
| Review specialists (58) | openai/gpt-6.1-sol |
1.3M | $2.8416 |
| Review | openai/gpt-6.1-sol |
28.7k | $0.0578 |
| Simulation | openai/gpt-6.1-sol |
14.9k | $0.0349 |
| Scoring | openai/gpt-6.1-sol |
6.1k | $0.0089 |
Dashboard → · React 👎 on an inline comment to dismiss it · Reply to an inline comment or use @argus-eye help
🧠 Learned: 2 finding memories · 1 PR summary · 1 architecture note
| return False | ||
|
|
||
| path = root | ||
| for part in rest.split(".") if rest else []: |
There was a problem hiding this comment.
Suggestion: _resolve reads the lazy entry before taking self._lock and never re-checks it. If another thread runs register_program_type(..., overwrite=True) or unregister_program_type('cirq') during the import, neither takes this lock. The later self._data[alias] = program_type then undoes that change. The window is narrow (not run). Fix: re-read self._data.get(alias, _MISSING) under the lock, raise KeyError if missing, return it if it's no longer a _LazyType, and only then load and publish.
There was a problem hiding this comment.
Fixed in 0e07656. _resolve now re-reads the entry under self._lock. It raises KeyError if the alias is gone, and returns the entry if it is no longer lazy.
The re-read alone left one gap. The import runs under the lock, but __setitem__ and __delitem__ did not take it, so an overwrite or unregister made during the import was still undone when the import published. Those two, clear and the update fast path now take the lock. A write made during an import waits for it, then applies.
test_write_during_lazy_load_is_kept(overwrite and remove) andtest_lazy_load_rechecks_entry_after_waiting_for_lockfail without the change. With it they pass, 15 runs in a row.tox -e unit-tests: 3569 passed, 113 skipped.tox -e format-checkpasses.
_resolve read the lazy entry before taking the lock and never re-read it, and __setitem__/__delitem__ did not take the lock. A register_program_type (..., overwrite=True) or unregister_program_type call made while another thread imported the entry was undone when that import published its result. _resolve now re-reads the entry under the lock: it raises KeyError if the alias is gone and returns the entry if it is no longer lazy. Writes take the same lock, so a write made during an import waits for it and then applies. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…imports A ModuleNotFoundError raised from qbraid's conversion code was skipped quietly whenever the missing module was outside qbraid. That is right for an SDK that is not installed, but an installed SDK without the submodule qbraid imports is an incompatible version, and its conversions vanished with no message. Skip quietly only when the SDK's top-level package is not found; otherwise skip with the usual warning. Raising instead would break import qbraid.transpiler for namespace packages such as braket, where the top-level package can be present without the submodule. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary of changes
import qbraid.programsimported every installed quantum SDK just to build the program type registry. The transpiler,load_programand every provider paid that cost. The registry is now lazy: an SDK is listed when it is installed and imported only when its type is used.Pain points fixed
load_programon a QASM string imported 7 SDKs. These came through the wrapper loops ingate_model,analogandannealing.Import time:
import qbraid.programsOne fresh venv per extra, Python 3.13. Each value is the mean of 5 warm runs, excluding interpreter start-up.
qbraid.runtime.awsdrops from 1.32 s to 0.43 s, andqbraid.runtime.pasqalfrom 1.21 s to 0.43 s.import qbraidis unchanged at about 110 ms.What changed
_import.py:LazyTypeRegistrymaps each alias to a module and class name. It checks that a package exists without importing it, and imports the class on first lookup.alias_manager.py: type detection checks only already-imported SDKs. The answer is the same. The "no match" error now lists alias names.registry.py:QPROGRAM_NATIVEis built when read. Register and unregister scan only loaded entries.gate_model,analog,annealing: wrapper modules load on attribute access.Verification
requirements-dev.txtinstalled.mainin all 23 venvs.['pennylane', 'dict']. This also happens onmainand will be fixed separately.Follow-ups
ConversionGraph()still imports SDKs whose conversion modules load them at the top of the file. With cirq installed it still takes 1.75 s.🤖 Generated with Claude Code
https://claude.ai/code/session_01MnewW15ZyxcusY61HhPBCU
Also in this PR:
NATIVE_REGISTRYandQPROGRAM_REGISTRYto mappings andQPROGRAM_TYPESto aMutableSetrather than concretedictandsettypes.QPROGRAM_NATIVEavailable through lazy attribute access.gate_model,analogandannealingto leave SDK wrappers unloaded; wrappers remain available by name.Auto-enriched by Argus