-
Notifications
You must be signed in to change notification settings - Fork 13
feat: Allow importing from model data to python. #2581
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
a08ae90 to
98f5bcd
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2581 +/- ##
==========================================
+ Coverage 82.97% 82.99% +0.01%
==========================================
Files 254 254
Lines 48021 48022 +1
Branches 43532 43532
==========================================
+ Hits 39845 39854 +9
+ Misses 6106 6098 -8
Partials 2070 2070
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
98f5bcd to
6a8a6ab
Compare
6a8a6ab to
edbf03f
Compare
|
I think this is working correctly as far as it goes, but its usefulness is currently limited by: |
| Ok(package) | ||
| } | ||
|
|
||
| #[pyfunction] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small docstring would help
hugr-py/rust/lib.rs
Outdated
| } | ||
|
|
||
| #[pyfunction] | ||
| fn model_to_json(bytes: &[u8]) -> PyResult<Vec<u8>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not essential but I think it would be better to instead bind
fn convert_envelope(bytes: &[u8], output_config: EnvelopeConfig) -> PyResult<Vec<u8>>; for more general purpose use, like in the cli:
Line 51 in 91bff6e
| pub fn run_convert(&mut self) -> Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, but unfortunately pyo3 doesn't know how to bind an EnvelopeConfig. I'm sure this could be fixed but not sure it's worth it for this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at least worth renaming to to_json_envelope or similar then I think, since the input can actually be any format
| case EnvelopeFormat.MODEL | EnvelopeFormat.MODEL_WITH_EXTS: | ||
| msg = "Decoding HUGR envelopes in MODULE format is not supported yet." | ||
| raise ValueError(msg) | ||
| # TODO Going via JSON is a temporary solution, until we get model import to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be good to link to an issue here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added link to #2287 ; will update the description there to clarify that this is the remaining task.
hugr-py/tests/test_envelope.py
Outdated
| assert op2.visibility == "Private" | ||
|
|
||
|
|
||
| @pytest.mark.xfail(reason="https://github.com/CQCL/hugr/issues/2589") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally wait to merge #2590 for the fix to merge (imminent) to avoid this xfail
hugr-py/Cargo.toml
Outdated
|
|
||
| [dependencies] | ||
| bumpalo = { workspace = true, features = ["collections"] } | ||
| hugr = { version = "0.22.4", path = "../hugr" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import from hugr_core directly
ss2165
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cheers
🤖 I have created a release *beep* *boop* --- ## [0.14.0](hugr-py-v0.13.1...hugr-py-v0.14.0) (2025-10-13) ### ⚠ BREAKING CHANGES * BorrowArray::{borrow, is_borrowed} return types have been swapped such that the array is first. ### Features * Add more options to `DotRenderer` config ([#2540](#2540)) ([45f7573](45f7573)) * Allow importing from model data to python. ([#2581](#2581)) ([4fb0a5e](4fb0a5e)) * **py, core, llvm:** add `is_borrowed` op for BorrowArray ([#2610](#2610)) ([1cd08ef](1cd08ef)), closes [#2569](#2569) ### Bug Fixes * Preserve offset for CFG edges when serializing to JSON ([#2606](#2606)) ([69a126d](69a126d)) ### Code Refactoring * consistent inout order in borrow array ([#2621](#2621)) ([8fc59f3](8fc59f3)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: Seyon Sivarajah <[email protected]>
Towards #2287 .
This is a temporary workaround to get the functionality now; it should be replaced with a proper implementation that parses the model data directly without going via JSON.