Uh oh!
There was an error while loading. Please reload this page.
Add guard to reject apply_to_subgraphs=True for self-subgraph-handling transforms - #240
Open
auphelia wants to merge 1 commit into
Open
Add guard to reject apply_to_subgraphs=True for self-subgraph-handling transforms#240auphelia wants to merge 1 commit into
auphelia wants to merge 1 commit into
Conversation
…forms that handle subgraphs internally
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Some transformations manage their own subgraph traversal because they need hierarchical context that the generic
apply_to_subgraphsdescent cannot provide (it re-enters each subgraph as a standalone top-level model, losing theparent hierarchy). Driving such a transform via
transform(..., apply_to_subgraphs=True)silently produces wrong results.This adds an opt-in marker and a fast-fail guard:
handles_subgraphs_internally = True.ModelWrapper.transformraises a clearValueErrorif it is invoked withapply_to_subgraphs=True, directing the caller to configure subgraph handlingon the transform itself.
The guard uses
getattr(transformation, "handles_subgraphs_internally", False), so all existing transforms (which don't set the marker) are completely unaffected.Motivation
Downstream (FINN),
ApplyConfigtraverses subgraphs itself using hierarchical config keys and cannot be driven by the generic mechanism. This guard turns the easy-to-makemodel.transform(ApplyConfig(cfg), apply_to_subgraphs=True)mistake into a loud error instead of silent misconfiguration.Changes
src/qonnx/core/modelwrapper.py: fast-fail guard at the top oftransform().tests/core/test_subgraph_traversal.py: newSelfHandlingTransformplus tests that the guard raises underapply_to_subgraphs=True(before anyapply()runs) and that the transform still works top-level-only otherwise.Note: I ran pre-commit on the PR as well, and I think it changed some lines that are not related to my changes.