Keep output specs in sync in ExportedProgramPassBase - #22864
mcremon-meta wants to merge 1 commit into
Conversation
Summary: A pass that replaces the graph's output node leaves `output_specs` naming the node that is no longer there. Nothing catches it at the time; it surfaces later as a signature that disagrees with the graph, and every pass author has to remember to fix it up by hand. `ExportedProgramPassBase.__call__` now realigns the specs with the graph between the pass and the postcondition check: ``` self.requires(exported_program) res = self.call(exported_program) signature_modified = _sync_output_specs(res.exported_program) self.ensures(res.exported_program) ``` Rewriting a spec to name the current output node is always safe. Changing an output between a node and a literal is not, so `_sync_output_specs` raises rather than guessing, and a length mismatch between outputs and specs raises too -- a pass that adds or removes an output is expected to maintain its own signature. Two consequences worth calling out: - A pass that only renames an output now reports `modified=True` even if it returned False, because the signature did change. That is what callers driving a pass to fixpoint need to see. - `ensures()` now receives the pass's result rather than the program that went in. The previous behaviour looks like an oversight -- a postcondition check that inspects the pre-pass program cannot check much -- and no pass in the tree relies on it: of the subclasses of `ExportedProgramPassBase`, exactly one overrides `ensures()`, and it calls `exported_program.validate()`, which wants the result. This was previously implemented as a `fused_quant`-local subclass. It is not specific to that package, so it moves here; the subclass goes away in the diff above this one. Differential Revision: D120259092
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22864
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New FailureAs of commit e6e2618 with merge base d65efde ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@mcremon-meta has exported this pull request. If you are a Meta employee, you can view the originating Diff in D120259092. |
This PR needs a
|
Summary:
A pass that replaces the graph's output node leaves
output_specsnaming thenode that is no longer there. Nothing catches it at the time; it surfaces later
as a signature that disagrees with the graph, and every pass author has to
remember to fix it up by hand.
ExportedProgramPassBase.__call__now realigns the specs with the graph betweenthe pass and the postcondition check:
Rewriting a spec to name the current output node is always safe. Changing an
output between a node and a literal is not, so
_sync_output_specsraisesrather than guessing, and a length mismatch between outputs and specs raises
too -- a pass that adds or removes an output is expected to maintain its own
signature.
Two consequences worth calling out:
modified=Trueeven if itreturned False, because the signature did change. That is what callers
driving a pass to fixpoint need to see.
ensures()now receives the pass's result rather than the program that wentin. The previous behaviour looks like an oversight -- a postcondition check
that inspects the pre-pass program cannot check much -- and no pass in the
tree relies on it: of the subclasses of
ExportedProgramPassBase, exactly oneoverrides
ensures(), and it callsexported_program.validate(), which wantsthe result.
This was previously implemented as a
fused_quant-local subclass. It is notspecific to that package, so it moves here; the subclass goes away in the diff
above this one.
Differential Revision: D120259092