Save the etrecord the lowered paths already generate - #22303
Open
shoumikhin wants to merge 2 commits into
Open
Conversation
`--generate_etrecord` produced no file and no warning on the Core ML and XNNPACK llama paths.
The flag was not being ignored, which is the part worth knowing: both helpers set
`generate_etrecord` on the builder, `to_edge_transform_and_lower` builds the record from it, and the
record travels all the way to the ExecuTorch program. Nothing ever saved it. So the cost was already
being paid, including a deepcopy of the edge program, and the artifact was dropped at the end.
One helper now writes it, from the same `export_program` the combined path uses and to the same
`etrecord.bin`, so all three paths leave the same artifact. A missing record is the normal case and
stays silent.
Also removed the TODO asking for exactly this.
One difference worth stating: the record from these paths is about twice the size of the combined
path's, because `to_edge_transform_and_lower` also records the aten exported program, which the
combined path does not. Measured on the default llama config, 1.65 GB against 826 MB for an 826 MB
`.pte`. That is content, not waste, but it is a size a user will notice.
Test plan:
Two tests driving the real XNNPACK helper, so they fail on the missing file rather than on a missing
symbol:
base FAILED, AssertionError: Lists differ: [] != ['etrecord.bin']
head 2 passed
Also ran the full llama export on the default config with only this file swapped:
base ['m.pte']
head ['etrecord.bin', 'm.pte']
and confirmed the written record loads: `parse_etrecord` returns an ETRecord with its edge dialect
program set.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22303
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (1 Unrelated Failure)As of commit d361bd7 with merge base c27baa8 ( BROKEN TRUNK - The following job failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
shoumikhin
added
the
release notes: examples
Two problems with the first version, both found in review.
The record was written inside the lowering, before the model was saved, and the write was not
guarded. So a failed record write took the whole export with it. Measured with an unwritable
target: the export raised and left no .pte, where the same run on the previous revision produced
one. A record about twice the size of the model makes a full disk the likely trigger, and losing a
model to a debug flag is the wrong trade.
The record is now written once, after `save_to_pte`, and a write error is logged and swallowed. That
also removes the duplicate call from the two lowering helpers.
The multimethod path dropped the record too. It builds one exactly as the other paths did and saved
only the .pte, so it gets the same call.
Test plan:
Three tests driving the real export with a stubbed builder:
saves the record base ['tiny.pte'], head ['etrecord.bin', 'tiny.pte']
writes none when unasked ['tiny.pte']
keeps the model when the record cannot be written .pte present, warning logged
The first fails on the previous revision, so it pins the fix rather than the helper.
Also corrected the docstring, which described the state before the change in the present tense and
claimed both paths leave the same artifact. They do not: this one carries the aten exported program
as well, so it is roughly twice the size.
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 free
to 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.
Closes #22301.
Summary
--generate_etrecordproduced no file and no warning on the Core ML and XNNPACK llama paths.The flag was not being ignored, which is the part worth knowing. Both helpers set
generate_etrecordon the builder,builder.pypasses it intoto_edge_transform_and_lower,_program.pybuilds the record from it, and the record travels all the way to the ExecuTorchprogram. Nothing ever saved it. So the cost was already being paid, including a
deepcopyof theedge program, and the artifact was dropped at the end.
One helper now writes it, from the same
export_programthe combined path uses and to the sameetrecord.bin, so all three paths leave the same artifact. A missing record is the normal case andstays silent. The
TODOasking for exactly this is removed.One difference worth stating
The record from these paths is about twice the size of the combined path's, because
to_edge_transform_and_loweralso records the aten exported program (add_exported_program), whichthe combined path does not. Measured on the default llama config:
That is content, not waste, but it is a size difference a user will notice, so it should be a
deliberate choice rather than a surprise. Happy to trim it to match the combined path if reviewers
prefer parity over completeness.
Test plan
Two tests driving the real XNNPACK helper, so they fail on the missing file rather than on a missing
symbol:
The second test asserts the not-asked case writes nothing, so the helper cannot start producing
stray files.
Also ran the full llama export on the default config with only
export_llama_lib.pyswapped:and confirmed the written record loads:
parse_etrecordreturns anETRecordwith its edge dialectprogram set.
Not covered
The Core ML helper takes the same one-line change as XNNPACK and is exercised by the same plumbing,
but I could not run a real Core ML lowering end to end here, since it completes only on macOS with
coremltools and the llama Core ML job does not run on a pull request touching only this file. The
XNNPACK path is what I measured.