Save the etrecord the lowered paths already generate - #22303
Conversation
🔗 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. ❌ 3 New Failures, 15 PendingAs of commit 958270e with merge base b213481 ( NEW FAILURES - The following jobs have failed:
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
|
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 |
6e6b9db to
a438b6e
Compare
|
Second pass, all four addressed. The record now goes where the model goes. I argued last time this was better left alone because The helper exported twice. On the tests barely failing at the merge base: two of the three now fail there, not one. The The Core ML claim is gone from the description. You were right that the arm is unreachable: the Also corrected the commit messages, which still described the first revision: one helper as the |
4e88698 to
a02d092
Compare
| path = os.path.join(builder.output_dir, "etrecord.bin") | ||
| try: | ||
| etrecord.save(path) | ||
| except Exception as error: | ||
| logging.warning("Could not write %s: %s", path, error) | ||
| return |
There was a problem hiding this comment.
Good catch, and fixed. An output name ending in .pte is used verbatim, so the model landed in the working directory while the record went to the output directory. The record now follows the saved model path, with the output directory as the fallback before anything is saved. Verified both ways: with output_name "m.pte" both files land in the working directory, and with "m" both land under the output directory.
a02d092 to
bcda29f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
examples/models/llama/export_llama_lib.py:1234
_save_etrecord_if_generatedalways writesetrecord.binunderbuilder.output_dir, but the.pteoutput path can be overridden via--output_name(including a full/relative path ending in.pte) which bypassesoutput_dirin_get_output_filename/save_pte_program. In that case the model and record end up in different directories. Consider deriving the etrecord directory from the actual saved.ptefilename (available viabuilder.get_saved_pte_filename()) so the artifacts stay colocated.
path = os.path.join(builder.output_dir, "etrecord.bin")
try:
etrecord.save(path)
except Exception as error:
logging.warning("Could not write %s: %s", path, error)
| if edge_manager_copy: | ||
| generate_etrecord_func( | ||
| et_record="etrecord.bin", | ||
| edge_dialect_program=edge_manager_copy, | ||
| executorch_program=builder.export_program, | ||
| ) | ||
| logging.info("Generated etrecord.bin") | ||
| et_record_path = os.path.join(builder.output_dir, "etrecord.bin") | ||
| try: | ||
| generate_etrecord_func( |
There was a problem hiding this comment.
Right, the description was stale. It described the behaviour before the path change, and it has been updated to say the record goes beside the model rather than into the working directory.
|
Done, on every path now. The backend-specific lowerings already worked that way, so the only holdout was the combined one, which built the record by hand with the standalone call and a deep copy of the edge manager. I had assumed converting it meant a wider change, but the plumbing was already there: to_edge takes the flag, to_backend and to_executorch both carry the record forward, and get_etrecord returns it at the end. The only gap was the two shared export helpers between the builder and to_edge, which did not pass the flag on. Adding it to both, defaulted off, leaves their other callers alone. That collapses the save helper to one branch and removes the deep copy and the attribute the lowering used to hand the record over. Two things worth knowing. Nothing covered the combined path before, and it needed covering: my first attempt set the flag at the lowering instead of before the edge export, and that path then produced no record at all while every other test still passed. There is a test for it now, and removing that one line fails it. And the record from that path is about twice the size, 8.5 MB to 17 MB on a small stack, because asking the program for it also gets the aten exported program that the hand-built one left out. That is what the other paths already produced. I also checked the memory: peak is the same either way, so the deep copy was not costing what its comment implied and this is about having one mechanism rather than about memory. Thanks for the pointer, this is better than what I had. |
c127914 to
dfbe9e9
Compare
`--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.
A shared helper now writes it, from the same `export_program` and to the same `etrecord.bin` name the
combined path uses. 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 larger than the combined path's, because
`to_edge_transform_and_lower` also records the aten exported program, which the combined path does
not. Roughly twice the size on the same model. That is content, not waste, but it is a size a user
will notice.
Test plan:
Tests driving the real export rather than the save helper, so they fail on the missing file rather
than on a missing symbol:
base the record is absent
head the record is written beside the model
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.
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 the 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.
Review found the record ignored the output directory. The model honoured `--output-dir` and the record did not, so with that flag set the larger of the two artifacts landed in the shell's working directory, and a script collecting the requested directory missed it. Both write sites now join the builder's output directory, so the two cannot disagree. I had argued this was better left alone because the combined path already wrote a bare name. That was the wrong call: the builder carries the directory and both sites already had it in hand, so fixing both is smaller than explaining why neither is fixed. Also removed a redundant export from the test helper. `_export_llama` exports the builder it is given, so returning an already exported one traced the model twice per test.
…there Three problems with the previous revision, all found in review. The record was written under `export.output_dir`, but an output name ending in `.pte` is used as the path verbatim and so bypasses that directory. With both set, which is the documented form in this repo, the model and its record landed in different directories. Measured on the combined lowering: before this stack both files landed together in the working directory, and after it the model was in the working directory and the record was in the output directory. The record path is now derived from the path the model was actually written to. The record was written in place. The format truncates its target when it opens it and closes in a finally, so a failure part way through left a short file under the real name and destroyed any record already there, reporting only a warning. Worse, the leftover still loaded: the parser accepted a 35479 byte remnant of a 36011 byte record. The record is now staged under a temporary name in the same directory and moved into place, so a failed write leaves the previous record untouched. The combined lowering still wrote its own record before the model, so a full disk could take the model and leave only the record. It now hands the edge program to the same post-model step, which leaves one write site and one rule. Also drop the function-local `import os` that the module-level one added here made dead. Test plan: The three earlier tests pinned none of the three fixes they were named for, because they patched out the only caller of `set_output_dir`, so the builder's output directory was always the default and the join could not be observed. They now point the builder at a temporary directory instead of changing the process working directory, which both makes the directory observable and removes a process-wide `chdir` from a suite that runs under `pytest -n auto`. Five tests now, each checked by reverting the behaviour it is named for: record path back to output_dir -> 1 failure guard narrowed from Exception to OSError -> 1 error staged write replaced by in-place write -> 1 failure record call removed from multimethod -> 1 failure Full file: 16 tests, 10 pass and 6 skip for a backend that is not installed here. Linux x86-64, Python 3.12.
The combined lowering was the one path still building the record by hand, with the standalone generator and a deep copy of the whole edge manager. Every other path asks the program for the record the export attached and saves that. The plumbing for the same thing was already there, just not connected: `to_edge` takes the flag, `to_backend` and `to_executorch` both carry the record forward, and `get_etrecord` returns it at the end. The two shared export helpers between the builder and `to_edge` did not forward the flag, so this adds it to both, defaulted off, which leaves their other callers unchanged. That makes the save helper one branch instead of two and removes the deep copy along with the attribute the lowering used to hand the record over. One thing worth knowing about the resulting file. Asking the program for the record gets the aten exported program as well, which the hand-built one did not carry, so the record from this path is now about twice the size for the same model. That is more to debug with, and it matches what the other paths already produced. Test plan: A new test drives the combined lowering, which nothing covered before, and checks the record lands beside the model and loads back. It is not decoration: the first version of this change set the flag at the lowering rather than before the edge export, and that path silently produced no record at all. Removing that one line now fails the test. Seventeen tests pass. Measured on a stack of eight linear layers, peak Python memory is the same either way, 30.8 MiB, so the deep copy was not costing what its comment implied and this is about having one mechanism rather than about memory. The record from that model goes from 8.5 MB to 17 MB, which is the aten program being included.
dfbe9e9 to
fba1fb7
Compare
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.
Four behaviours worth calling out, all 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.
hands the record to the same post-model step instead, so there is one write site and one rule.
was the last that did it by hand, with the standalone generator and a deep copy of the edge
manager.
to_edgealready accepted the flag and bothto_backendandto_executorchalreadycarried the record forward; the two shared export helpers in between did not pass the flag on, so
this adds it to both, defaulted off, leaving their other callers unchanged.
export.output_dir. An output name ending in.pteis used as the path verbatim, so it bypassesoutput_dir, and keying the record offoutput_dirwould put the pair in two different directories for the documented combination ofexport.output_dirplus a.pte-suffixedexport.output_name.target when it opens it, so writing in place turned a failure part way through into a short file
under the real name, destroying any previous record while reporting only a warning.
One difference worth stating
Asking the program for the record also gets the aten exported program, which the hand-built one on
the combined path did not carry. So that path's record is now roughly twice the size for the same
model: measured 8.5 MB before and 17 MB after on a stack of eight linear layers. That is more to
debug with, and it is what the other paths already produced. Peak Python memory is unchanged at
30.8 MB either way, so removing the deep copy is about having one mechanism rather than about
memory.
Test plan
Six tests, driving the real export with the model preparation stubbed:
output_dirpoints elsewhere; both files still land together['tiny.pte'], and no warning logged.ptepresent, warning logged, no stray file['etrecord.bin', 'tiny.pte'], and it loads back['etrecord.bin', 'tiny.pte']Each test was checked by reverting the behaviour it is named for and confirming it fails:
output_dirExceptiontoOSErrorThe combined lowering test is the one that earned its place: the first version of that change set
the flag at the lowering rather than before the edge export, and that path then produced no record
at all while every other test still passed.
The tests point the builder at a temporary directory rather than changing the process working
directory, because the suite runs under
pytest -n autowhere the working directory is shared.Not covered
lowering.
those paths still produces no file and no warning. That predates this change and is left alone
here rather than wired up on paths this change does not touch.