Save the etrecord the lowered paths already generate - #22303
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 6e6b9db 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.
Review found the guard did not hold where it was written down. The docstring said the save is never allowed to raise, but only OSError was caught. Injecting a RuntimeError from the save showed it escaping the export after the .pte was already on disk, which is the exact outcome the change exists to prevent. Widened to Exception, which is what save_pte_program on the line above already does. The combined lowering was the third path and still wrote its record before the model, unguarded, so a failed write there lost the export. That write is older than this change, but it breaks the same rule, so it now warns and continues too. Three test corrections: The positive test only checked the file name, so a zero byte record passed. It now loads the record back and asserts the edge program, which fails when the save is replaced by an empty file. The failure test asserted only that the .pte survived, which is also true on the previous revision where nothing wrote a record. It now asserts the warning it is named for. Its docstring claimed an unwritable directory loses the record and not the .pte. Measured: in a read only directory both are lost and the export still reports success, because the model save swallows its own error. Dropped that half. Also removed a dynamic shape line and its comment from the test setup. Validation rejects dynamic shapes only for Core ML and QNN, and these tests enable XNNPACK, so nothing read the flag. Hoisted torch and the builder import to module scope to match the sibling test files.
|
Thanks, this was a useful pass. Fixed and pushed: The docstring promised the save never raises but only The combined lowering was the third path and still wrote its record before the model, unguarded. It Three test corrections. The positive test only checked the filename, so a zero byte record passed; it Also removed the dynamic shape line from the test setup. Validation rejects dynamic shapes only for Two I am not changing here, with reasons: The bare Inferring the flag from the attached record instead of reading it. You are right that the The description was rewritten against the current code, and the |
Summary
Asking for an etrecord on a lowered llama export produced no file and no warning. The record was
built, carried through to the ExecuTorch program, and then dropped.
to_edge_transform_and_lowerattaches the record when asked, but nothing saved it, so the cost ofbuilding it was paid and the artifact thrown away.
What changes
The record is saved once, after
save_to_pte, from the two export entry points. Three paths reachit now: the XNNPACK lowering, the Core ML lowering, and the multimethod export, which dropped its
record the same way.
Two behaviours worth calling out, both added after the first revision:
unwritable target took the whole export down and left no
.pteat all. A record roughly twice thesize of the model makes a full disk the likely trigger, and losing a model to a debug flag is the
wrong trade.
older than this change but broke the same rule, so it now warns and continues too.
One difference worth stating
The record from the lowered paths carries the aten exported program, which the combined path's does
not, so it is roughly twice the size for the same model. Same filename, more content.
Test plan
Three tests, driving the real export with the model preparation stubbed:
['tiny.pte'], head leaves['etrecord.bin', 'tiny.pte']['tiny.pte'].ptepresent, warning loggedThe first is the regression test: it fails on the previous revision. The third fails if the guard is
removed. The first also loads the record back with
parse_etrecord, because asserting the filenamealone passes for a zero byte file.
Not covered
lowering.
a backend field that no longer exists. That is a separate fix, and until it lands the Core ML arm
of this change is unreachable.
etrecord.binin the working directory, so it ignores--output-dirwhile the model honours it. That matches the combined path's existing behaviour, soit is left alone here rather than changed on one path only.