fix: unwrap qir bitcode wrappers before qir-qis conversion#161
fix: unwrap qir bitcode wrappers before qir-qis conversion#161
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes build failures when QIR bitcode is provided via wrapper objects (with a .bitcode attribute) by unwrapping before calling qir_qis conversion/validation, and adds regressions to cover wrapped inputs.
Changes:
- Add a helper to unwrap
.bitcodewrappers and use it in QIR bitcode kind matching and QIR→QIS conversion. - Ensure QIR validation/conversion receives raw
bytesrather than wrapper objects. - Add tests to ensure wrapped QIR and wrapped lowered-QIS bitcode can be built successfully.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
selene-sim/python/tests/test_build_classification.py |
Adds regression tests that build from wrapped QIR and wrapped translated QIS bitcode. |
selene-core/python/selene_core/build_utils/builtins/qir.py |
Introduces _unwrap_bitcode_resource(...) and uses it to unwrap wrappers before QIR validation/conversion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _unwrap_bitcode_resource(resource: Any) -> bytes | None: | ||
| if hasattr(resource, "bitcode"): | ||
| resource = resource.bitcode | ||
| return resource if isinstance(resource, bytes) else None |
There was a problem hiding this comment.
QIRBitcodeStringKind.matches now accepts wrapper objects with a .bitcode attribute, but not all steps that consume QIRBitcodeStringKind unwrap the resource. In particular, QIRBitcodeStringToQIRBitcodeFileStep.apply still calls out_path.write_bytes(input_artifact.resource), which will raise a TypeError if the resource is a wrapper. Consider unwrapping there as well (e.g., via _unwrap_bitcode_resource) to keep wrapper support consistent across the QIR bitcode pipeline.
| validate_qir(input_artifact.resource) | ||
| result = qir_to_qis(input_artifact.resource) | ||
| bitcode = _unwrap_bitcode_resource(input_artifact.resource) | ||
| assert bitcode is not None, "QIR bitcode step received a non-bitcode resource" |
There was a problem hiding this comment.
Avoid using assert for validating runtime inputs here. Assertions can be disabled with python -O, which would skip the check and produce a less clear downstream failure in validate_qir(...)/qir_to_qis(...). Prefer raising a regular exception (e.g., TypeError/ValueError) when _unwrap_bitcode_resource(...) returns None.
| assert bitcode is not None, "QIR bitcode step received a non-bitcode resource" | |
| if bitcode is None: | |
| raise TypeError("QIR bitcode step received a non-bitcode resource") |
|
Unneeded, there was a regression in qir-qis with the inkwell 0.9 upgrade that's resolved in Quantinuum/qir-qis#72 |
Summary
qir_qisbytesand LLVM IR text inputsRoot Cause
QIRBitcodeStringKindaccepted wrapper-style inputs with a.bitcodeattribute, but the QIR-to-Helios step still passed the wrapper object itself intovalidate_qir(...)andqir_to_qis(...). That madebuild(BitcodeString(qir_bc))fail even though the same artifact worked as plainbytes.Source Of Regression
This behavior was introduced in PR #114 (
feat: QIR support using QIR-QIS, commit70ab294), which added direct QIR support together with theBitcodeString-compatible matcher and the QIR-to-Helios conversion step. The matcher unwrapped.bitcode, but the conversion step still forwarded the wrapper object itself intoqir_qis.PR #157 is related context but not the introducing change. That patch fixed QIR-vs-Helios classification for lowered
qir-qisoutput; it did not address this wrapper-handling gap in the QIR conversion step.Validation
uv run pytest selene-sim/python/tests/test_build_classification.py -quv run pytest selene-sim/python/tests/test_qir.py -q