Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2772 +/- ##
========================================
Coverage 83.81% 83.81%
========================================
Files 267 269 +2
Lines 53660 53953 +293
Branches 47748 47920 +172
========================================
+ Hits 44975 45223 +248
- Misses 6275 6315 +40
- Partials 2410 2415 +5
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:
|
There was a problem hiding this comment.
This is pretty good, I particularly like your implementation of "scope" :)
- However there are issues about addition of new nodes?
- Wrt. recursive and flat regions....so let's think about the two sorts of passes:
- those that operate on flat regions. These can additionally recurse on sub-containers, kinda separately, and this is roughly what you get by calling
regions. Note this means that when operating on a region, such passes must not change any container, as they're gonna recurse on it later. (I guess that also means making sure they continue to feed equivalent inputs to the container - so every container/subregion-parent is a black box.) Note thatrecursiveisn't really a question that such a pass would ask, it just looks atregions. - those that operate on subtrees. In which case these are gonna call
rootsand then consider all/some descendants. (Note special case whenrootsincludes a function and the entrypoint is inside it, I'm not sure what we can do about that.) This is the case whenrecursiveis required. Perhaps insteadrootsshould also return a bool ?
- those that operate on flat regions. These can additionally recurse on sub-containers, kinda separately, and this is roughly what you get by calling
Finally, with respect to the breakage....
- This is breaking now (we're adding a non-default method to the trait). Ok, this isn't so bad given we're about to make a breaking release, but it seems particularly galling that we still leave unsolved #2771. Well, fair, that is a bunch of work - so I we'll roll out "fixes" to individual passes as non-breaking releases, fine :). But then "breaking" 0.26.0 will just change the doc that passes "must" follow the trait method doc? So downstream crates will "upgrade" to a newer hugr-passes in order to assert that they themselves follow that doc?
This means that consumers get the pain now (and let's say they open issues to "properly implement with_scope before 0.26.0, just like we do here). Then at the point when implementing that method and solving the issue is actually required....there's no noise, just a change to a doc; we rely on them to track that from the first issue.
- The alternative, rather than adding 10 do-nothing methods, is a trait default method - so this is non-breaking. We could roll this out later (after 0.25.0), maybe even with the first batch of passes using it, say. Then breaking change (0.26.0) removes the default - at which point downstream crates will actually break if they haven't implemented
with_scopethemselves.
I guess downstream crates would get less warning that they need to do something, so it might be longer before they update to 0.26.0. But feels to me that (if we assume downstream crates follow our example) we'll get a stronger guarantee that they have actually done the right thing for 0.26.0.
- Or.... Leave ComposablePass as per 0.24.0, and define a new trait ComposablePassWithScope that's the same but with the new method. Blanket-implement CPWithScope for any ComposablePass, with the do-nothing method, and deprecate ComposablePass, for removal in 0.26.0. I guess that's "the right way"; if this is a big enough concern that we wanna (1) get the new method out there ASAP, (2) give a large window of opportunity for implementation, (3) have a breaking change when it becomes required....is it worth it?
hugr-passes/src/scope.rs
Outdated
| @@ -0,0 +1,384 @@ | |||
| //! Scope configuration for a pass. | |||
| //! | |||
| //! This defines the parts of the HUGR that a pass should be applied to, and | |||
There was a problem hiding this comment.
I reckon you could probably do the module doc with just a one-liner that says "see [PassScope]". I'm not sure whether "Scope configuration" really tells people anything (recognition of a term once they've seen it before), either, as we haven't really defined what "Scope" is, so you could drop that.
hugr-passes/src/scope.rs
Outdated
| /// The scope of a pass defines which parts of a HUGR it is allowed to modify. | ||
| /// | ||
| /// Each variant defines the following properties: | ||
| /// - `roots` is a set of **regions** in the HUGR that the pass should be |
There was a problem hiding this comment.
Consider "flat regions" rather than just "regions"?
Has the term "regions" been precisely defined somewhere else? If not, we should do so here. I believe the correct term from the spec is "Dataflow sibling graph"...
There was a problem hiding this comment.
Also, some optimizations like nesting CFGs (perhaps even simpler ones like CFG normalization) cannot necessarily be expressed as operations on flat regions.
There was a problem hiding this comment.
Given only EntrypointFlat really depends on the notion of regions, perhaps this should not be a main/defining property of a PassScope but just a helper method for passes that care.
There was a problem hiding this comment.
Yeah, having written main PR comment, and noting you have both/separate fn regions and fn roots - I think the latter is the correct approach, whereas the doc here defines roots as being the regions.
hugr-passes/src/scope.rs
Outdated
| let Some(fn_op) = hugr.get_optype(node).as_func_defn() else { | ||
| return false; | ||
| }; | ||
| let public = fn_op.visibility() == &Visibility::Public; |
There was a problem hiding this comment.
nit: either call this is_public or just inline at its single point of use
hugr-passes/src/scope.rs
Outdated
| if node_parent != hugr.module_root() { | ||
| return true; | ||
| } | ||
| // For module children, only private functions |
There was a problem hiding this comment.
Maybe "Additionally allow removing/modifying private functions" ?
hugr-passes/src/scope.rs
Outdated
| /// - `scope` is a set of **nodes** in the HUGR that the pass **MAY** modify. | ||
| /// - This set is closed under descendants, meaning that all the descendants | ||
| /// of a node in `scope` are also in scope. | ||
| /// - Nodes that are not in `scope` **MUST** remain unchanged. |
There was a problem hiding this comment.
both in terms of their signature/optype and their behaviour...or clarify scope is the nodes which may be modified both as ops and as the behaviour defined by their descendants
hugr-passes/src/scope.rs
Outdated
| /// | ||
| /// Nodes in `root` are never in scope, but their children will be. | ||
| /// | ||
| /// If [PassScope::recursive] is `true`, then all descendants of nodes in |
There was a problem hiding this comment.
This suggests that if recursive is false, then descendants of nodes in root are not in scope, i.e. MUST NOT be modified? (EDIT: Yes, that is consistent with elsewhere, so ok.)
There was a problem hiding this comment.
Hmmm. IIUC, you can't modify anything outside of roots. So if you run with AllPublic you can't modify the private functions?? EDIT sorry yes, the private functions are still in_scope even though not in roots.
EDIT2 So roots is really more about goals/targets than scope. How about renames...
scope -> may_modify and/or may_modify_children? (/may_modify_descendants - perhaps one might even make that false if the thing contains the entrypoint, but true for all non-entrypoint descendants, although "modify_children" might mean, "ok to add children")
roots -> target_roots
|
Urgh. Tried to use this for RemoveDeadFuncsPass....ok, that is an awkward one, maybe should have tried something simpler.
and got this error: which sounds a bit like it isn't using the call to You can see this in 6ed9ee6 |
|
Ok how about: |
I note this is only really one enum variant more than we have now, i.e. very similar! But I think this allows to summarize:
|
If the entrypoint is the module-root, there are a few possibilities for "preserve semantics of only the entrypoint"....
|
hugr-passes/src/composable/scope.rs
Outdated
| /// Run the pass on the whole Hugr, but preserving behaviour only of the entrypoint. | ||
| /// | ||
| /// If the entrypoint is the module root, then the same as [Self::Public]. | ||
| // ALAN could be Self::All in such case, but if so then Self::Public |
There was a problem hiding this comment.
This was meant as a clarification to reviewers, I'll remove it before merging
There was a problem hiding this comment.
Don't forget to remove this
hugr-passes/src/composable/scope.rs
Outdated
| pub fn from_entrypoint(h: &impl HugrView) -> Self { | ||
| if h.entrypoint() == h.module_root() { | ||
| // EntrypointXX say not to do anything in this case, so pick a global pass (rather arbitrarily) | ||
| Self::Global(Preserve::All) // ALAN or default? |
There was a problem hiding this comment.
This is to help with backwards compatibility with legacy pass interfaces, so might be better left to the next PR. (Since passes do not currently take visibility into account, Preserve::All is probably appropriate/non-breaking, even if not what one would want generally.)
There was a problem hiding this comment.
Update: I will remove this function before merging, but it's here FYI so you can see "how bad it is". (However, one can argue it's only bad because this is what we'd need to preserve behaviour of legacy passes, i.e. it's only bad because the legacy behaviour is so annoying, and we will eventually remove that in a breaking change...)
| class ABCEnumMeta(ABCMeta, EnumMeta): | ||
| """Custom metaclass for things that inherit from both `ABC` and `Enum`. | ||
|
|
||
| This is to solve the error: | ||
| `Metaclass conflict: the metaclass of a derived class must be a (non-strict) | ||
| subclass of the metaclasses of all its bases [metaclass]` | ||
| """ |
There was a problem hiding this comment.
Using Protocol rather than ABC would simplify things here.
(We use the former in the rest of the codebase)
There was a problem hiding this comment.
While Protocol allows to remove the @abstractmethod decorator, it has the same problem with metaclasses (when combined with Enum), and moreover, I had problems that "Protocol cannot be instantiated" (again I think because of combining with Enum). If anyone can do better, please feel free :) :)
hugr-py/tests/test_scope.py
Outdated
|
|
||
|
|
||
| @pytest.fixture | ||
| def test_hug() -> TestHugr: |
There was a problem hiding this comment.
Intentional typo?
There was a problem hiding this comment.
Kinda, but there's no good reason not to call it test_hugr, so done that
acl-cqc
left a comment
There was a problem hiding this comment.
Agustin, myself and Callum all approve, so never mind who's PR it is or who wrote the code, that seems like enough!
Follow-up to #2772 with non-breaking updates to ConstFoldPass, DeadCodeElimPass, NormalizeCFGsPass, RedundantOrderEdgesPass, RemoveDeadFuncsPass, ReplaceTypes, UntuplePass. (Breaking updates to follow in #2871.) Generally this means keeping pre-existing pass configurating methods but deprecating, and storing such config in an `Either<PassScope, old-config>`, so we can drop the old config in time. Also deprecated toplevel functions `constant_fold_pass`, `remove_dead_funcs` and `monomorphize`. --------- Co-authored-by: Agustín Borgna <[email protected]> Co-authored-by: Agustín Borgna <[email protected]>
## 🤖 New release * `hugr-model`: 0.25.6 -> 0.25.7 (✓ API compatible changes) * `hugr-core`: 0.25.6 -> 0.25.7 (✓ API compatible changes) * `hugr-llvm`: 0.25.6 -> 0.25.7 (✓ API compatible changes) * `hugr-passes`: 0.25.6 -> 0.25.7 (✓ API compatible changes) * `hugr-persistent`: 0.4.6 -> 0.4.7 (✓ API compatible changes) * `hugr`: 0.25.6 -> 0.25.7 (✓ API compatible changes) * `hugr-cli`: 0.25.6 -> 0.25.7 (✓ API compatible changes) <details><summary><i><b>Changelog</b></i></summary><p> ## `hugr-model` <blockquote> ## [0.25.6](hugr-model-v0.25.5...hugr-model-v0.25.6) - 2026-02-20 ### New Features - Remove size limitation for binary envelopes ([#2880](#2880)) </blockquote> ## `hugr-core` <blockquote> ## [0.25.7](hugr-core-v0.25.6...hugr-core-v0.25.7) - 2026-03-06 ### Documentation - added examples in docs srtring ([#2920](#2920)) </blockquote> ## `hugr-llvm` <blockquote> ## [0.25.6](hugr-llvm-v0.25.5...hugr-llvm-v0.25.6) - 2026-02-20 ### New Features - Add error context when lowering hugrs to LLVM ([#2869](#2869)) </blockquote> ## `hugr-passes` <blockquote> ## [0.25.7](hugr-passes-v0.25.6...hugr-passes-v0.25.7) - 2026-03-06 ### Documentation - added examples in docs srtring ([#2920](#2920)) ### New Features - Define pass application scopes ([#2772](#2772)) - Modify dead code elimination pass to remove unreachable basic blocks ([#2884](#2884)) - Add non-generic `with_scope` method for composable passes ([#2910](#2910)) - update passes to use PassScope where non-breaking ([#2836](#2836)) </blockquote> ## `hugr-persistent` <blockquote> ## [0.4.0](hugr-persistent-v0.3.4...hugr-persistent-v0.4.0) - 2025-12-22 ### New Features - [**breaking**] Remove `RootCheckable` ([#2704](#2704)) - [**breaking**] Bump MSRV to Rust 1.89 ([#2747](#2747)) - [**breaking**] Type-safe access for node metadata ([#2755](#2755)) ### Refactor - [**breaking**] Remove multiple deprecated definitions ([#2751](#2751)) </blockquote> ## `hugr` <blockquote> ## [0.25.7](hugr-v0.25.6...hugr-v0.25.7) - 2026-03-06 ### Documentation - added examples in docs srtring ([#2920](#2920)) ### New Features - Define pass application scopes ([#2772](#2772)) - Modify dead code elimination pass to remove unreachable basic blocks ([#2884](#2884)) - Add non-generic `with_scope` method for composable passes ([#2910](#2910)) - update passes to use PassScope where non-breaking ([#2836](#2836)) </blockquote> ## `hugr-cli` <blockquote> ## [0.25.6](hugr-cli-v0.25.5...hugr-cli-v0.25.6) - 2026-02-20 ### New Features - Add s expression format to envelope formats ([#2864](#2864)) </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/release-plz/release-plz/).
🤖 I have created a release *beep* *boop* --- ## [0.15.5](hugr-py-v0.15.4...hugr-py-v0.15.5) (2026-03-16) ### Features * Define pass application scopes ([#2772](#2772)) ([847b864](847b864)) * deprecate Function (Value) in hugr-py too ([#2882](#2882)) ([043fec2](043fec2)) * Improve rendering of tags and cases ([#2943](#2943)) ([6ba9e45](6ba9e45)) ### Bug Fixes * **py:** Load and resolve lower_funcs in extensions ([#2924](#2924)) ([6dfc8e3](6dfc8e3)) ### Documentation * Add specification to sphinx docs ([#2907](#2907)) ([46ea04b](46ea04b)) --- 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: Agustín Borgna <[email protected]>
Adds support for `PassScope`s in the pass definitions. See Quantinuum/hugr#2772. ~Requires a hugr release with the `PassScope` definition and Quantinuum/hugr#2910 ~Many of these also call other passes from `hugr_passes`. While we pass the scope config along, proper support requires Quantinuum/hugr#2836 and Quantinuum/hugr#2871 This is a rust-only change, the python interface will follow up. BREAKING CHANGE: Multiple unit-like pass structs must now be constructed using a `::default()` call instead. BREAKING CHANGE: `QSystemPass` is now a `ComposablePass`. Import the trait to call `run`. BREAKING CHANGE: `QSystemPass` no longer implements `Copy`. BREAKING CHANGE: Renamed `tket_qsystem::extension::qsystem::lower_tket_op` to `lower_tket_ops`.
Defines
PassScopeconfigurations in bothhugr-passesandhugr-pythat let users define set the parts of the hugr that a pass should optimize.Since each pass must be carefully modified to support the new config, this PR only adds the definitions (and a
ComposablePass::with_scopemethod), and states that passes are not required to follow the configuration (for now).This will let us make the required changes incrementally as non-breaking changes. I opened an issue to track the implementations. #2771
Closes #2748. See discussion about pass properties there. We left the
NamedFunctionsvariant to be defined later.Python tests included adding optional
visibilityparam toModule.define_functionanddeclare_function.