[shoumikhin]

Related: pytorch/TensorRT#5045 uses this to move its compiled engines into the program.

What is wrong today

FileBackedData keeps a large blob, such as a delegate's compiled engine, in a file until the program is written. Writing it copies that file into the .pte or .ptd. Until the caller deletes the file afterwards, the blob is on disk twice. For an engine of tens of gigabytes, that copy alone can make an export run out of disk.

Separately, ExecutorchProgramManager.save and save_pte_program catch a failed write, log it and return normally, so a caller can end up with a broken file and no error.

What this change does

Commit 1: save and save_pte_program raise when a write fails. save_pte_program now writes the .pte through save. Behavior change: the legacy ExecutorchProgram, which the deprecated exir.capture(...).to_edge().to_executorch() path returns, has no save, so save_pte_program no longer accepts it (its type hint already says ExecutorchProgramManager).

Commit 2: a keyword-only consume=True on save and write_tensor_data_to_file. This is new public API. With it, data that owns its file (cleanup=True, as from move_from) is moved into the output instead of copied:

• Chunks are copied from the end back to the start, and the source is truncated after each chunk is written, flushed and synced. The part not yet written stays a hole, so the disk holds the moved data once, plus one chunk. On a file system without sparse files the peak is the same as a copy.
• The cost is time: the sync per chunk makes a move several times slower than a copy (3 to 9 times in two measurements on btrfs). It buys a disk peak of about one copy of the data instead of two.
• The output is a temporary file in the destination directory, renamed onto the final name only on success. A failed or killed save leaves the final name as it was. If data was already moved, a note names the temporary file, which holds it; otherwise the temporary file is deleted. A symlink at the final name is followed and the file it names keeps its mode; a hard link at the final name is replaced, as any rename replaces it.
• Moved data can't be read or written again, so any ETRecord has to be saved first. Both methods check, before they open anything, that every buffer still holds its bytes and that the output is not the data's own file.
• Buffers shared between files, or repeated in one, are copied. A deep copy of a FileBackedData is the same object, so a program lowered from a deep-copied edge program can still move its data.

write_to_file methods, which take a file the caller opened, always copy, as before. Anything that can't be moved safely is copied too: data that does not own its file, a link, a file with another hard link or that is not writable, a source on another file system, and Windows. The save also falls back to a plain copy, exactly as without consume, when the final name is not a new or writable regular file (a pipe, for example) or when no temporary file can be created next to it.

Other API changes: FileBackedData.check() and FileBackedData.path, Cord.file_backed_buffers(), and reading a FileBackedData whose file changed size raises.

Example:

et = to_edge_transform_and_lower(exported, partitioner=[...]).to_executorch()
et.save("model.pte", consume=True)  # owned delegate files move into model.pte

Tests

exir/program/test/test_program.py and exir/_serialize/test/test_cord.py, on Linux x86_64. Each commit's tests of new behavior fail on the code before it; a few tests in commit 2 check behavior that must not change, and pass on both. They cover a test backend, lowered with to_edge_transform_and_lower, whose data is moved into the .pte or .ptd; the disk peak during a move (allocated blocks must stay under 1.5 times the data, where a copy needs 2 times; 64 MiB of data in 4 MiB chunks, so the holes APFS fills with zeros, up to about 16 MiB, still fit); a save stopped partway; writing again after a consuming write; writing over the data's own file; every copy fallback, checked on both the source and the output; and failures at each step of the move, with the notes checked against the bytes on disk. The tests of the move itself are skipped on Windows, which always copies.

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/25099

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 2b2d2e5 with merge base b3b565b ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[shoumikhin]

@claude review effort=max

[claude]

Claude finished @shoumikhin's task in 28m 14s —— View job

---

Request changes: the first write to reach a shared FileBackedData consumes it, and save() hides the error this causes. The move can also lose data with several kinds of output, and whenever it fails partway through.

PR Review: #25099

Summary

The PR adds a consume_on_write option to FileBackedData. With it on, the data is moved into a seekable output from the end back to the start, and the source is truncated as it goes.

The main problem is the design: buffer, ETRecord and repeated saves all share this object, and it now has a destructive, single-use mode. The move itself can also lose data:

• it accepts outputs where writes don't land at the offset it seeks to;
• it truncates the source before the output has the chunk;
• a failure can't be retried or recovered.

How this was checked: by reading the code and the CPython stdlib sources. The job wasn't allowed to run Python, so nothing was executed. pytorch/TensorRT#5045 was not reviewed.

Backward Compatibility

• The mode is set on a shared object, and the first write triggers it (exir/_serialize/_cord.py:33, :95-101).
  • Why the object is shared: the backend sets consume_on_write when it creates the object.
    • NamedDataStore stores the object itself (exir/_serialize/_named_data_store.py:174).
    • LoweredBackendModule.__deepcopy__ passes named_data_store_output by reference (exir/lowered_backend_module.py:122).
    • ExecutorchProgramManager builds _pte_data once (exir/program/_program.py:1939) and reuses it.
  • What breaks: once a backend such as Torch-TensorRT turns the flag on, these user flows break after the first write into a seekable file:
    • et.buffer raises (_program.py:2022 → Cord.__bytes__ → to_bytes()).
    • generate_etrecord(...) with the lowered edge program raises in serde (exir/serde/serialize.py:55-56, :308).
    • A second write_to_file() raises partway through, after the header and earlier segments are already written.
    • save() catches Exception and only logs it (_program.py:2067-2072). Calling et.save(p) twice reopens p with "wb", which truncates the good .pte. The call writes the header, hits the RuntimeError, and returns normally. The result is a corrupt .pte, the engine is gone from both files, and nothing is raised.
  • Ownership is inverted: move_from (_cord.py:51-64) creates the files the program owns, such as the CUDA backend's blobs (backends/cuda/cuda_backend.py:1136, backends/cuda/cuda_weight_collector.py:125). It can't set the flag. Meanwhile, a cleanup=False file that the object doesn't own can be consumed.
  • Fix: this answers the API question in the description. Take the alternative you proposed:
    • put the opt-in on the write (Cord.write_to_file, ExecutorchProgramManager.write_to_file, save);
    • apply it only to buffers that own their file (cleanup=True);
    • say in those docstrings that the program can't be written or read again afterwards;
    • make save() re-raise.
  • Timing: decide this before fix(executorch): move compile_and_save's engines into the program instead of copying them TensorRT#5045 lands, because that PR makes another repo depend on this keyword.
  • Fix this →

Code Quality

• The output check lets through outputs where writes don't land at the seeked offset (_cord.py:95-99). seekable() plus the mode string doesn't prove the output is a regular file that accepts writes at any offset.

  • O_APPEND descriptor opened with mode "wb" (for example os.fdopen(os.open(p, O_WRONLY | O_CREAT | O_APPEND), "wb"), or sys.stdout.buffer under >> out.pte):

    • FileIO.mode comes from the mode string, so the check passes, but every chunk lands at the end of the file.
    • Blobs over 8 MiB end up with their chunks in reverse order. The total size is right and nothing is raised, but the data is corrupt and the source is gone.

  • gzip.GzipFile (an io.BufferedIOBase): seekable() always returns True.

    • On 3.11/3.12, mode is the int 2, so "a" not in ... raises TypeError. A copy would have worked.
    • On 3.13+, mode is "wb". The first seek pushes size - chunk zeros through the compressor. The tail chunk is written and cut from the source. Then the next seek raises OSError("Negative seek in write mode"), and the tail chunk is lost.

  • io.BytesIO and open(os.devnull, "wb"): both pass (BytesIO has no mode), so writing into memory or doing a dry run destroys the source.

  • Writers that only implement write(): they now fail with AttributeError on seekable().

  • Fix: move only when all of these hold, and copy otherwise:

    • the output is an io.BufferedWriter, io.BufferedRandom or io.FileIO;
    • os.fstat(fileno()) reports a regular file (S_ISREG);
    • on POSIX, fcntl(F_GETFL) has no O_APPEND.

    This also replaces the getattr(outfile, "mode", "") probe (CLAUDE.md: no dynamic getattr patterns). Fix this →

• The source is truncated before the code knows the chunk reached the output (_cord.py:119-121). The return value of outfile.write(chunk) is not checked, and nothing flushes before source.truncate(begin).
  • Buffered output: io.BufferedWriter holds a write that fits in its buffer. It only reaches the kernel at the next seek(), which runs after the truncate.
    • This always applies to the data's first chunk (size % 8 MiB bytes, written last) when it fits in the buffer.
    • On 3.14+, open() sizes the buffer as max(min(st_blksize, 8 MiB), 128 KiB). On filesystems that report a large st_blksize, full chunks can be held too.
    • If the delayed flush fails, or the process dies in between, those bytes are in neither file. A flush can fail with ENOSPC, EDQUOT or EIO, for example when the output is on another filesystem and the truncate freed nothing there.
  • Unbuffered output (open(p, "wb", buffering=0), a seekable FileIO): a partial write on a full disk returns a short count instead of raising.
    • The source is truncated anyway.
    • The next write succeeds in the space the truncate just freed, so the output silently contains zeros.
  • Fix: before each truncate, check that write() returned len(chunk) and call outfile.flush(). Fix this →
• A failed move can't be retried or recovered (_cord.py:107-122, with the message at :72).

  • Before any truncation: self._moved = True is set before the first read or write.

    • Any error at this stage leaves the source intact, but this object can never read it again. An example is ENOSPC on the first outfile.write, which is the low-disk case this PR targets.
    • The error message says the data "was moved", which is false.
    • The user can't free space and call save() again.

  • After the first truncation: an exception or Ctrl-C leaves [0, end) in path and [end, size) in an output the caller will delete.

    • Nothing records end.
    • With cleanup=True, the finalizer deletes the part left in path.
    • save() hides the failure (see above).
    • The copy path left the source intact in every one of these cases.

  • Fix:

    • mark the data consumed only right before the first truncate;
    • after a truncate, raise an error that says how many bytes are left in path and where the rest is in the output;
    • document in the class docstring what a failed consuming write leaves behind.

    Fix this →

• The "consumed" state is stored on the object, not on the file (_cord.py:39, :69-74, :102-103).
  • Trigger: another FileBackedData on the same path, such as a second construction or a copy.copy taken before the move, sees an empty file:
    • to_bytes() and prefix() return b"", and sha256() hashes nothing.
    • Its copy path writes 0 bytes while __len__ still reports the original size. Every later offset in the .pte shifts, silently.
  • Coverage: the length check at :115-118 only guards the move path, and that is all test_cord.py:150-160 tests.
  • Fix: check os.fstat(f.fileno()).st_size == self._size in _open. That covers every object on the path and replaces _moved, which also fixes the early-flag problem above. Fix this →
• Truncating a file is not the same as deleting it (_cord.py:107). The docstring describes the option as being for "a caller that deletes path", but open(path, "r+b") behaves differently from a delete:
  • Links: it follows symlinks, and truncation affects every hard link to the file. If the engine is a link into a cache, the cached copy is destroyed. os.remove would only drop one name.
  • Permissions: it needs write permission. A read-only source (mode 0444, or a read-only mount) could still be copied, but now the write fails, and inside save() that failure is only logged.
  • Fix: copy instead when os.lstat(path) isn't a regular file with st_nlink == 1, or when the r+b open fails. Limiting the move to files created by move_from, as suggested above, also rules out both cases. Fix this →

Testing

• The main move test doesn't measure what it claims (test_cord.py:96-118):
  • Disk use: on_disk adds the bytes passed to write() to the source's apparent size. It doesn't count allocated blocks. So the test can't show that the unwritten part stays a hole, and it would pass unchanged on NTFS (see Performance).
  • Write order: chunk = 1000 is smaller than io.BufferedWriter's buffer (8 KiB, or 128 KiB on 3.14+).
    • Every chunk reaches Recording.write at the next seek, after its source range has already been truncated.
    • The + chunk slack in the bound is never used.
    • The test passes without the missing flush, and it would still pass if the code truncated before writing.
  • Fix: measure st_blocks * 512 for both files, and have Recording.write assert that the source still holds the range being written.
• Missing tests:
  • a writer that fails partway through a move, checking the source, the output and the object afterwards, and a retry;
  • writing the same Cord twice, or calling bytes(cord) after a move;
  • ExecutorchProgramManager.save() followed by .buffer or a second save().
• Append test: test_cord.py:135-148 doesn't check that the source is still intact after the copy, unlike the pipe test at :133.

Fix this →

Performance

• No disk saving, plus an extra full-size write, where the output can't be sparse (_cord.py:119-120, docstring :19-22).

  • Why: the first write lands at start + size - chunk. These filesystems allocate the whole gap at once, before any source byte is freed:

    • NTFS, unless the file is marked sparse, which Python never does;
    • FAT/exFAT;
    • HFS+.

    NTFS and exFAT also fill the gap with zeros.

  • Result: peak disk use is twice the data size, the same as a copy. On top of that, size bytes of zeros are written, and the source is still destroyed.

  • Who is affected: Windows is a supported CUDA export target (.ci/scripts/wheel/test_cuda_windows.py), and Torch-TensorRT supports Windows.

  • Fix: copy instead when os.name == "nt", and limit the docstring's "never on disk twice" to filesystems with sparse files. Fix this →

Specific Comments

• exir/_serialize/_cord.py:17: the summary line "stays on disk until explicitly closed" is no longer true when consume_on_write is on.
• exir/_serialize/_cord.py:100,105,109: "move" now means two different things in one class. move_from renames the file, while _move_to_file and _moved copy and truncate. The public flag says "consume", so _consume_into and _consumed would avoid the clash.
• exir/_serialize/test/test_cord.py:94: the written = [0] list used as a counter would read more clearly as a nonlocal variable or an attribute on Recording.

Recommendation

Request Changes

What blocks approval:

• With the constructor flag, the first write can destroy data that buffer, ETRecord and later saves still need, and save() turns that into a silently corrupt .pte.
• Moving the opt-in to the write call, as the description already proposes, fixes that.
• Before the move is safe to turn on for multi-gigabyte engines, it also needs a strict output check, a flush before each truncate, and defined behavior when it fails.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[shoumikhin]

@claude review effort=max

[claude]

Review of PR #25099

Request changes: after a consuming save, a second or retried save truncates its output before it notices the data is gone, which destroys the only copy, and a failure partway through a program with several owned buffers leaves no record of where the moved ones went.

PR Review: #25099

Summary

The PR adds an opt-in consume=True to FileBackedData.write_to_file, Cord.write_to_file and ExecutorchProgramManager.write_to_file/write_tensor_data_to_file/save. With it, owned file-backed data is moved into the output instead of copied: chunks go back to front, and the source is truncated behind each one. The PR also makes save re-raise write errors.

The main problems:

• a second or retried write destroys data;
• the failure guarantees hold per buffer, not per program;
• the move has a symlink hole and a signal race;
• the docstrings don't state what callers are left with, or when the move saves no space.

Python commands aren't permitted in this job, so the tests weren't run. Everything below comes from reading the code.

Code Quality

• A second or retried write after a consuming one wipes the moved data. Major.
  • save (_program.py:2090) and write_tensor_data_to_file (:2065) open the output with "wb", which truncates it, before any FileBackedData checks its size in _open (_cord.py:76-83).
  • Trigger: et.save("model.pte", consume=True) succeeds, then et.save("model.pte") runs again, for example in a script that saves twice. The second call empties model.pte, writes the leading bytes, and then _open raises. The delegate data is now in neither file.
  • The natural response to a failed consuming save does the same thing. Retrying it to the same path wipes the bytes the note says are "at offset N of the output".
  • Fix: before opening the output, check that every file-backed buffer in the cords being written still holds all its bytes. The second call then raises without touching the file.
  • Fix this →
• The eligibility check misses symlinks. Major impact, uncommon trigger.
  • os.lstat (_cord.py:117) describes the link, not its target, so a link passes the st_nlink != 1 guard whatever its target's link count.
  • _open("r+b") then follows the link and truncates the target. The object never owned the target; cleanup only ever deletes the link.
  • Trigger: FileBackedData.move_from(path) where path is a symlink, because os.replace moves the link itself. For example, a file in a Hugging Face hub snapshots/ directory links into the shared blobs/ store, and the consuming save empties the shared blob.
  • Fix: also require stat.S_ISREG(source.st_mode). Comparing the opened descriptor's st_dev/st_ino with the lstat result also closes the gap between the check and the open.
  • Fix this →
• A Ctrl-C during truncate leaves the bookkeeping one chunk behind (_cord.py:148-149).
  • CPython runs pending signal handlers as soon as a call returns. A KeyboardInterrupt that arrives during source.truncate(begin) is therefore raised before end = begin runs.
  • The note then reports [begin, end) as still in the source, but those bytes are only in the output.
  • On the first chunk it is worse. end == self._size, so the handler adds no note and leaves the finalizer alive. The source still holds everything except the last chunk, and it is deleted when the object is collected.
  • Fix: assign end = begin before the truncate. The note is then true whether or not the truncate happened.
  • Fix this →
• A write error reported late can lose a chunk without a note. Suspected.
  • flush() (_cord.py:147) only hands the chunk to the kernel.
  • NFS, common for home and scratch directories on clusters, and many FUSE filesystems report ENOSPC/EDQUOT late: on a later write, on fsync, or on close(). By then truncate has already dropped the source's copy.
  • If the error comes at close() (the with in save), _consume_into has already returned, so there is no note at all. If it comes on a later chunk, the note lists bytes as being in the output that never got there.
  • A crash between the truncate and writeback has the same effect on a local disk.
  • Fix: call os.fsync(outfile.fileno()) before each truncate, which is one call per 8 MiB chunk. Otherwise, document that the move isn't safe on such filesystems or across crashes.
  • Fix this →
• Some outputs make the check raise instead of falling back to a copy. Minor.
  • outfile.fileno() (_cord.py:122) raises io.UnsupportedOperation for a BufferedWriter over a raw stream with no descriptor, such as io.BufferedWriter(io.BytesIO()).
  • _open("r+b") (:133) can still fail after os.access has passed.
  • Both errors propagate, although the docstring promises that "any other output … gets a copy".
  • Fix: copy when these raise OSError before anything has been moved. UnsupportedOperation is a subclass of OSError.
  • Fix this →
• A buffer that appears twice in one cord is moved at its first occurrence. Minor, latent.
  • set(cord.file_backed()) (_program.py:2077) drops repeats within a cord, so they never reach keep. Cord.write_to_file doesn't handle repeats either, so the second occurrence raises in _open after the first one emptied the file.
  • The in-tree serializer doesn't produce repeats today: delegate segments are deduplicated at exir/_serialize/_program.py:298-302, and named data is deduplicated by hash. The comment at _program.py:2072-2073 gives the reason repeats would matter.
  • Fix: drop the set(), and document in Cord.write_to_file that a repeated buffer must be in keep.
  • Fix this →

API Design

• The failure guarantees hold for one buffer, not for a program (_cord.py:29-31, :234-238, _program.py:2048-2050).
  • Both the note and the promise that the data "can be written again" describe a single FileBackedData.
  • Take a cord with two owned buffers where the first is fully moved. If the second then fails, the error carries at most the second buffer's note, and no note at all if it fails before its own first truncation.
  • Nothing says that the first buffer's bytes now exist only at some offset in the partial output, or that the program can't be written again.
  • Deleting the broken .pte or retrying the save (the first Code Quality finding) then loses those bytes. For the tens-of-gigabytes engines that motivate this PR, that means rebuilding them.
  • Fix: have Cord.write_to_file add a note that lists the buffers already moved and their offsets in the output, state this program-level contract in the ExecutorchProgramManager.write_to_file docstring, and test two owned buffers where the second fails.
  • Fix this →
• The post-condition at _program.py:2048-2050 claims less, and more firmly, than what happens. Minor.
  • The buffers belong to the lowered modules, not to this manager. et gets them through the edge manager's NamedDataStore (:1864), and LoweredBackendModule.__deepcopy__ shares them (exir/lowered_backend_module.py:122).
  • So after et.save(path, consume=True), the lowered EdgeProgramManager can't be serialized either. A second to_executorch() from it can't be saved, and generate_etrecord given the lowered manager fails in _NamedDataEncoder (exir/serde/serialize.py:56). _data_in_several_files only looks at this manager.
  • In the other direction, nothing is moved on Windows or for any output that _can_consume_into rejects, and the program stays usable. Code that works on one machine can therefore break on another.
  • Fix: document both, and consider logging when the move falls back to a copy.
  • Fix this →

Backward Compatibility

• save now raises for every caller, not only for callers that pass consume (_program.py:2095).
  • Before this PR, a failed write was logged and swallowed. Raising is the right behavior, but it changes a public API. The PR's title, which is also its release notes: exir entry, only describes the opt-in move.
  • save now always passes consume= to self.write_to_file. An out-of-tree subclass that overrides write_to_file(self, open_file) therefore gets a TypeError from every save().
  • Fix: mention the save change in the title or the release note (Release notes: api, CONTRIBUTING.md:418), or move it to its own PR.

Testing

• Six of the new tests fail on Windows, and a seventh likely does, yet the Windows unit-test job runs them.
  • pytest-windows.ini:26 collects exir/_serialize/test. The unittest / windows job runs on every PR and is still running on this one.
  • On Windows the move always falls back to a copy, so:
    • test_consume_moves_owned_data_without_a_second_copy fails: there is no st_blocks, and the source isn't emptied.
    • test_only_a_consuming_write_consumes and test_another_buffer_on_a_consumed_file_refuses_to_copy_it fail because no RuntimeError is raised.
    • test_a_consuming_write_that_fails_first_keeps_the_data fails because shutil.copyfileobj ignores the short write.
    • test_a_consuming_write_that_fails_later_says_where_the_bytes_are fails because there is no note, and del data deletes the file.
    • test_consume_copies_data_it_cannot_write_to fails because there is no os.geteuid.
    • The O_APPEND case in test_consume_copies_into_outputs_it_cannot_move_into likely fails too. Without os.O_BINARY, os.open opens the file in text mode, so every \n in the payload is written as \r\n.
  • Fix: skipIf(os.name == "nt") on the tests that need the move, and O_BINARY on Windows.
  • Fix this →
• The late-failure test can't catch a wrong offset in the note (test_cord.py:245-269).
  • The output starts at offset 0, so start + end == end, and a note that dropped start would still pass.
  • The test also only matches the note's text. It never reads the output at the offset the note gives.
  • Fix: write a prefix first. Then check that the output holds _PAYLOAD[left:] at the offset in the note, and that the source holds _PAYLOAD[:left].
  • Fix this →
• No program-level test checks that a consuming save actually moves anything (test_program.py:1215-1269).
  • Two of the tests mock Cord.write_to_file on programs that have no file-backed data, so keep=set() is trivially true. The third uses a buffer that has to be copied anyway.
  • If _data_in_several_files returned every buffer, which turns the feature off, all three would still pass.
  • Fix: add a test where _pte_data holds an owned buffer that no .ptd shares, and check that save(consume=True) empties the source and writes a correct .pte. Ideally add a second one that goes through real lowering, since every program test sets _pte_data by hand.
  • Fix this →

Performance

• Across filesystems, or onto one without sparse files, the move saves no space but still consumes the program (_cord.py:21-26, :112-128).
  • When st_dev differs, each filesystem peaks just as it would with a copy: the source's at the start, the output's at the end. A typical case is engines written to a local /tmp or a tmpfs and the .pte saved to a home or NFS directory.
  • On exFAT, FAT or HFS+, the first write lands at the far end of the output and allocates the whole output, and some of these filesystems also zero-fill it. The peak is then a copy's, plus an extra pass of writes.
  • Only Windows is excluded today.
  • Fix: copy when os.fstat(fd).st_dev != source.st_dev (the fstat call is already there), and qualify "never on disk twice" (_cord.py:22).
  • Fix this →

Specific Comments

• _cord.py:80-83: the message is wrong when the file grew or was replaced, e.g. "holds 200 of its 100 bytes; … taken the rest". "Expected N bytes, found M" fits every case.
• _cord.py:157-160: the note doesn't name the output, which matters for write_tensor_data_to_file because it writes several files. "are still in it" would be clearer as "are still in {path}".
• _program.py:2067-2069: _data_in_several_files() is recomputed for every .ptd. Compute it once before the loop.
• _program.py:2071: _data_in_several_files reads as "data split across files". _shared_file_backed_data says what it returns.
• _program.py:2078: (shared if data in seen else seen).add(data) is clever. A plain if/else is simpler.

Recommendation

Request Changes. Four findings can each lose data that only a full re-export brings back, and each has a small fix:

• the destructive second write (check before opening the output);
• the symlink hole (reject symlinks);
• the signal race (set end before truncating);
• the program-level failure contract (add a note at the cord level).

The Windows test failures will also block CI.

• Load the /pr-review skill and the PR context (base origin/main, one commit, fcfc152)
• Three independent reviewers each go through the full diff (_cord.py, _program.py, test_cord.py, test_program.py)
• Run the tests: not possible here, because Python commands need approval in this job. The findings come from reading the code.
• Check which points from the previous review were addressed
• Merge duplicate findings and check each one against the code
• Post the review

View job run

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[shoumikhin]

@claude review effort=max

[claude]

Claude finished @shoumikhin's task in 40m 34s —— View job

---

PR Review: #25099

• Load the /pr-review skill and the PR context (base origin/main, one commit, 4 files)
• Three independent reviewers go through the full diff
• Merge duplicate findings and check each one against the code (by reading it: running Python, and so the tests, needed approval in this job)
• Check which points from the earlier reviews were addressed
• Post the review

Request Changes: a write after a consuming write is only caught after the output has been opened with "wb", so a second write to the same path (a re-run cell, a retry, save_pte_program) erases the only copy of the moved data, and there is still no test that goes through real lowering.

Detailed review

Summary

The PR adds consume=True to FileBackedData, Cord and ExecutorchProgramManager (write_to_file, write_tensor_data_to_file, save). With it, an owned file is moved into the output instead of copied: the file is written from the end back to the start and truncated after each synced chunk, so a large delegate payload is on disk only once.

• Main problem: write_to_file can't protect an output its caller reopens. A comment says every output is checked before it's opened, which isn't true for this method.
• Still open from an earlier review: a copy made under consume is silent, and no test goes through real lowering.
• Smaller problems:
  • A killed process leaves no record of where the moved bytes are.
  • Cord documents a precondition it could enforce.
  • The output check accepts wrappers.
  • There's an fsync per 8 MiB chunk.

Code Quality

• A killed process leaves no record of where the bytes are (minor), exir/_serialize/_cord.py:189-198.
  • The note on the error is the only record of a move that stopped partway, and it's only added when an exception reaches the except.
  • A SIGTERM (timeout, a cancelled CI job, a scheduler), a SIGKILL or the OOM killer during a long move gives no exception. The first bytes stay in a hidden .executorch_*.data file and the rest are in the output, and nothing records this.
  • Separately, save logs {e} (exir/program/_program.py:2126), and that leaves out __notes__.
  • Fix: log the source path, size and output offset once before the first truncation. In save, use logging.exception or drop the line, since the error is re-raised. Fix this →
• Cord.write_to_file documents a precondition it could enforce (minor), exir/_serialize/_cord.py:271-272.
  • If a caller leaves a repeated buffer out of keep, the first occurrence is moved and the second raises RuntimeError partway through the write. The partial output is then the only copy.
  • The Cord knows its own buffers. Counting self.file_backed() and copying any buffer that appears twice takes about two lines. _shared_file_backed_data would then only need to handle sharing across files. Fix this →
• The output check accepts wrappers that don't write where they seek (minor, low confidence), exir/_serialize/_cord.py:136.
  • isinstance accepts any BufferedWriter or BufferedRandom, whatever its raw is.
  • A buffered wrapper over a stream that transforms the data but reports the underlying file's fileno(), such as io.BufferedWriter(gzip.GzipFile(...)), passes every later check. The source is truncated after the first chunk, and then the backward seek raises.
  • A subclass that hashes what it writes receives the chunks from the end back to the start.
  • Fix: for the buffered types, also require isinstance(outfile.raw, io.FileIO). The Recording and FailsThird tests still pass with this. Fix this →

Testing

• No test goes through real lowering (raised in an earlier review, still open).
  • Every manager-level test in exir/program/test/test_program.py sets et._pte_data/et._tensor_data by hand.
  • So nothing checks that a backend's FileBackedData reaches the .pte/.ptd Cords intact through to_backend → to_executorch (the named data store, AlignedData(Cord(...)), segments), or that the consumed output still deserializes.
  • The only producers in the repo outside tests are in the CUDA backend (backends/cuda/cuda_weight_collector.py:125, backends/cuda/cuda_backend.py:1136), which needs CUDA.
  • A variant of BackendWithNamedDataMap (exir/backend/test/backend_with_named_data_map.py) whose preprocess stores FileBackedData.move_from(...) covers this on CPU, since add_named_data accepts a FileBackedData. The test would save with consume=True, then assert that the backing file is empty and that the deserialized .pte holds the payload. Fix this →
• exir/_serialize/test/test_cord.py:272-275: the retry after the early failure checks only the output bytes. It still passes if the retry silently copies, for example if the first failure had detached the finalizer. Assert that the retry returns 0 and leaves data._path empty. Fix this →
• exir/_serialize/test/test_cord.py:138 (low confidence): the peak is 25 blocks of 4 KiB and the bound is about 26. The test would fail on file systems that allocate in bigger units, such as tmpfs on 64 KiB-page kernels. Taking the slack from os.statvfs(directory).f_frsize avoids that.

API Design

• Reopening a consumed output erases the only copy (major), exir/program/_program.py:2067 and 2088-2090.
  • The comment says every buffer is checked "before its output is opened". That holds for save and write_tensor_data_to_file. It doesn't hold for write_to_file, whose caller has already opened open_file with "wb" before line 2067 runs.
  • Say a notebook cell or a retry loop runs this a second time:

    with open("model.pte", "wb") as f:
        et.write_to_file(f, consume=True)

    It truncates model.pte, which now holds the only copy of the moved payloads, and only then raises.
  • save_pte_program (extension/export_util/utils.py:146-152) does the same when it runs after a consuming save. It also swallows the RuntimeError and returns the path of an empty .pte; the only trace is a log line.
  • 67 files in the repo write programs with open(..., "wb") + write_to_file, so that is the code people will add consume=True to.
  • Fix:
    • Correct the comment.
    • Say in the write_to_file docstring that after a consuming write the output is the only copy and must not be reopened for writing. Point to save, which checks before opening.
    • Have save_pte_program call prog.save(filename) instead of opening the file itself. Fix this →
• A copy under consume is silent (raised in an earlier review, still open), exir/_serialize/_cord.py:121-125 and exir/program/_program.py:2063-2064.
  • Even when the data and the output are on the same file system, an owned buffer is copied instead of moved when:
    • the file has a second hard link;
    • the output is opened for append;
    • the output is a tempfile.NamedTemporaryFile (its wrapper isn't a BufferedWriter);
    • the file isn't writable;
    • the buffer is shared with another output;
    • the platform is Windows.
  • The data is then on disk twice, which is what consume is meant to avoid. Whether the program can still be written depends on which path each buffer took.
  • write_to_file and save return None either way, so the caller can't tell why the disk filled up or whether a second save is safe.
  • Fix: log a warning with the path and size when an owned buffer is copied under consume, or have the manager methods report whether anything was moved. Fix this →
• consume can be passed positionally (minor): exir/program/_program.py:2052, 2072 and 2108, and exir/_serialize/_cord.py:116 and 264.
  • et.save(path, True) moves data out of its file, but nothing in the call says so.
  • The flag is new, so making it keyword-only (*, consume: bool = False) breaks no callers. Fix this →

Performance

• One flush and fsync per 8 MiB chunk (magnitude not measured), exir/_serialize/_cord.py:167-188.
  • A 40 GiB payload means about 5,000 fsync calls, each waiting on the device (and a round trip on NFS or Lustre).
  • Reads and writes also run one after the other, where a copy overlaps them. So the move may be much slower than the copy it replaces.
  • Each truncation needs a sync before it, but there's no need to truncate after every chunk. Writing 256 MiB in 8 MiB pieces and then syncing and truncating once:
    • keeps memory at one chunk;
    • caps the extra disk use at 256 MiB;
    • cuts the number of syncs by a factor of 32.
  • Please benchmark a multi-GiB move against a copy. Fix this →

Specific Comments

• exir/_serialize/_cord.py:85: check() doesn't say what it checks. A name like check_not_consumed() would read better at its call site, exir/program/_program.py:2093.

• exir/_serialize/_cord.py:182: the comment says the chunk "has to be on the output's disk". On macOS, os.fsync doesn't flush the drive's write cache (fcntl.F_FULLFSYNC does), so after a power loss the truncation can survive while the chunk doesn't.

• exir/program/_program.py:2061-2062: the docstring says buffer can't be read after a consuming write. It still can if it was read before, because it's cached at lines 2032-2034.

• exir/program/_program.py:2096-2097: the comment covers only a buffer shared by the .pte and a .ptd. The loop also catches a buffer repeated within one file, which test_consume_copies_a_buffer_repeated_in_one_file relies on.

• exir/_serialize/test/test_cord.py:203-207: the seekable() override has no effect.

  • _open_to_consume never calls seekable().
  • On POSIX the pipe already fails the S_ISREG and st_dev checks.
  • On Windows the function returns before it looks at the output.

  The test name and comment describe a check the code doesn't have.

Recommendation

Request Changes

Two things block this PR:

• write_to_file can erase a consumed .pte, while a comment says that case is handled. The fix is small: the comment, the docstring, and sending save_pte_program through save.
• There's still no end-to-end test through real lowering.

The silent copy and the sync per chunk are open to discussion. But on a large model they decide whether consume actually keeps the data on disk once.

[shoumikhin]

@claude review effort=max

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[claude]

Claude finished @shoumikhin's task in 37m 13s —— View job

---

Review of PR #25099

Request Changes: if a consuming save is killed partway through, it leaves a full-length .pte that the runtime loads with zeros where data hadn't been moved yet, and consume still copies silently in cases the caller can't see, now including programs built from a deep copy of a lowered edge manager.

Detailed review

PR Review: #25099

Summary

The PR adds consume=True to FileBackedData.write_to_file and Cord.write_to_file. It moves file-backed data the program owns into the output from back to front, truncating the source after each synced chunk. consume is passed through ExecutorchProgramManager.write_to_file, write_tensor_data_to_file and save, and save and save_pte_program now raise on failure.

Request Changes, for two reasons:

• A consuming save that is killed leaves path at its full length, and it loads with zeroed data.
• consume still copies silently in cases the caller can't detect.

Points the previous review raised that are still open are marked (still open). I couldn't run the tests because running Python in this job needs approval, so every finding comes from reading the code.

Code Quality

• A killed consuming save leaves a .pte that the runtime loads, with zeros for the data not yet moved (major).

  • How it happens. _cord.py:167-188 writes each chunk straight to its final offset, starting with the buffer's last chunk, so the output has its full length after the first write. If the process dies before the move finishes, no handler runs. That covers SIGKILL, SIGTERM, the OOM killer, a CI timeout and a power cut. path keeps its full length, and the unwritten start of the buffer is a hole that reads as zeros. Those bytes now exist only in a hidden .executorch_*.data file that nothing points to.
  • Why the runtime accepts it. serialize_pte_binary adds no padding after the last segment (exir/_serialize/_program.py:480-496). So when the buffer being moved is the last segment, the file passes the runtime's only size check (expected <= actual, runtime/executor/program.cpp:110-119). The flatbuffer is written before the segments, so it verifies too.
  • What the user sees. A zeroed delegate header usually fails when the method loads. Zeroed named data, such as weights, gives wrong outputs with no error.
  • Compared with a copy. A killed copy leaves a short file that the runtime rejects, and the source is intact, so the save can be re-run. A killed move leaves neither.
  • .ptd files. Each .ptd from write_tensor_data_to_file (_program.py:2083-2088) has the same problem. When one fails, the notes don't say which .ptd holds the moved bytes.
  • Fix.
    • With consume, have save and write_tensor_data_to_file write to a temporary file in the destination directory.
    • os.replace it onto the final name only after the write succeeds.
    • Create the file with open(name, "xb") rather than mkstemp, so it gets the usual permissions instead of 0600.
    • On failure, name the temporary file in a note.
    • Before the move starts, log the sources and the temporary file, so a killed run leaves a trail.

  Fix this →

Testing

• test_program.py:1349-1368. The .ptd branch checks assertIn(payload, f.read()), which passes wherever the payload lands in the file. A move to the wrong offset in a .ptd would not be caught. Also, the 16 KiB payload moves as a single 8 MiB chunk, so this end-to-end test never runs the multi-chunk path. Fix: patch _COPY_CHUNK_SIZE down to a few KiB, save the same program without consume into a second directory first, and compare the .pte and .ptd byte for byte. Fix this →
• test_cord.py:213-231 (still open). _open_to_consume never calls seekable(), and an anonymous pipe already fails both the S_ISREG check and the st_dev check. So the ReportsSeekable override can't change the outcome, and the test can't fail for the reason its name gives. Fix: move the override to the output of the named-pipe test (test_cord.py:247), where S_ISREG is the only check that rejects it. Fix this →

API Design

• consume=True still falls back to a copy without telling the caller (still open).

  • Where. _open_to_consume (_cord.py:131-161) returns None on Windows and in these cases:

    • the object doesn't own the data;
    • the output isn't a regular file opened without O_APPEND;
    • the source has another name or is on another file system.

    Nothing above it reports the fallback: Cord.write_to_file, ExecutorchProgramManager.write_to_file and save all return None. A caller who passed consume because the disk can't hold two copies finds out when the disk fills.

  • A trigger the previous review didn't name.

    • EdgeProgramManager and NamedDataStore don't define __deepcopy__, so copy.deepcopy(edge_manager) deep-copies NamedDataStore.buffers.
    • Each copied FileBackedData gets a weakref.finalize that was never registered, so _finalizer.alive is False (_cord.py:133).
    • to_executorch() serializes from that store (_program.py:1871-1876).
    • So with a manager lowered by to_edge_transform_and_lower, calling to_executorch() on a deep copy, as docs/source/etrecord.rst:126-139 does, gives a program where save(consume=True) copies every buffer.

  • Fix. Log a warning once for each buffer that falls back, with the reason. Give FileBackedData a __deepcopy__ that returns self (the data on disk doesn't change, as with bytes), so deep copies keep ownership.

  Fix this →

• generate_etrecord() fails after a consuming save of a lowered program.

  • Why. generate_etrecord() reads every FileBackedData in the lowered modules of the edge manager it is given (exir/serde/serialize.py:52-55), and LoweredBackendModule.__deepcopy__ keeps the same objects (exir/lowered_backend_module.py:122).
  • Which flows.
    • docs/source/llm/export-custom-llm.md:521-535 deep-copies the lowered manager, calls to_executorch() on the original, then calls generate_etrecord() with the copy. If a consuming save runs before generate_etrecord(), it raises the RuntimeError from _check_size.
    • With the __deepcopy__ fix above, the etrecord.rst flow fails the same way.
    • The generate_etrecord=True flow isn't affected, because it records the programs from before lowering.
  • Fix. The write_to_file docstring (_program.py:2059-2068) should say to generate the ETRecord first.

  Fix this →

• Repeated buffers (still open).

  • Problem. _cord.py:272-273 requires callers to put every repeated buffer in keep, but Cord.write_to_file could find the repeats in self._buffers itself. A direct caller who forgets to do this hits the following:
    1. The first occurrence is moved.
    2. The second occurrence fails with the RuntimeError from _check_size.
    3. The only copy of the data is left in a half-written output.
  • Why ExecutorchProgramManager is safe. Only because _shared_file_backed_data (_program.py:2099-2110) also catches repeats within one file, which its comment doesn't mention.

  Fix this →

Performance

• One sync per 8 MiB chunk (still open). _cord.py:183-184 syncs the output after every _COPY_CHUNK_SIZE chunk. Moving a 4 GiB payload therefore waits on 512 device flushes in a row, and each one can take tens of milliseconds on network or cloud disks. I haven't measured this. Fix: give the move its own, larger chunk size, such as 64–256 MiB. That costs that much extra disk and a pread buffer of the same size. Then time a multi-GB move against the copy path. Fix this →

Specific Comments

• exir/program/_program.py:2061-2062 (still open): the docstring says the data can't be "read through buffer again", but that's wrong if buffer was read before the consuming write. It is cached at _program.py:2032-2033 and keeps returning those bytes.
• exir/program/_program.py:2129-2131 (still open): the except now only logs and re-raises. The log uses str(e), which drops the notes that say where the moved bytes are, and the error is reported twice. Drop the try/except.
• exir/_serialize/_cord.py:257: file_backed() reads like a predicate; file_backed_buffers() would say what it returns.

Fix this → (all three)

Recommendation

Request Changes. The killed-save problem under Code Quality blocks approval. Because the move writes in place, an interrupted consuming save becomes a .pte that loads with zeroed data, and the source has already given up those bytes. The silent fallback under API Design should be fixed in the same pass, or callers can't rely on consume to save disk space. Everything else is minor.

Review steps

• Load the /pr-review skill
• Read the diff against origin/main
• Run the three independent reviewers
• Try to run the tests. I couldn't: running Python here needs approval, so every finding comes from reading the code.
• Merge duplicate findings
• Check each finding against the code
• Check which points from the earlier reviews were addressed
• Post the review

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[shoumikhin]

@claude review effort=max

[claude]

Claude finished @shoumikhin's task in 44m 0s —— View job

---

PR Review: #25099

Request Changes: a consuming write that fails or is interrupted can't be retried (even for data it never moved), overwriting an existing output saves no disk space, and data that can't be moved is copied without notice; the other findings are minor.

Detailed review

Summary

The PR makes save and save_pte_program raise when a write fails. It also adds consume=True, which moves owned FileBackedData into the output chunk by chunk, truncating the source as it goes, through a temporary file that is renamed onto path. Three problems need changes: a failed consuming write can't be retried, a re-export over an existing file gets no disk saving, and the silent fallback from the earlier reviews is still open. The tests were not run (running Python needs approval in this job), so the findings come from reading the code.

API Design

• A consuming write that fails can't be retried, even for data it never moved.
  • write_tensor_data_to_file checks the buffers of every .ptd up front (exir/program/_program.py:2133). Say a.ptd is written, which empties its sources, and b.ptd then fails before moving anything. Every later write_tensor_data_to_file call raises at that check on a.ptd's buffers, so b.ptd's intact data can't be written at all.
  • A .pte save that fails after its first truncation can't be repeated either. save raises at 2204, with or without consume. The data is split between the source files and the .partial file, and the only way back is the byte offsets in the error notes.
  • The likely triggers are Ctrl-C during a save the docstring calls several times slower, and ENOSPC, which the overwrite finding under Performance makes more likely. In practice the user has to rerun the export.
  • Fix: on failure, move the data back into its sources, using the same bounded protocol in reverse from the end of the output. Write every .ptd to its temporary file before renaming any, so one failure rolls back all of them. If that is out of scope, the save and write_tensor_data_to_file docstrings should say that a failed consuming write can't be retried. Fix this →
• Data that can't be moved is copied without notice, and a save with nothing to move still goes through the temporary file. The silent fallback is still open from the earlier reviews.
  • _open_to_move (exir/_serialize/_cord.py:139-165) refuses a buffer by returning None. The only trace is the INFO line at _program.py:1927-1930, which is hidden by default. That line lists every buffer path (one per weight storage for the CUDA backend, backends/cuda/cuda_weight_collector.py:472) and says "moved where possible" without saying which buffers moved.
  • Example: the CUDA backend creates its files next to the AOTI .so (backends/cuda/cuda_backend.py:1166-1169). aot_compile gets no aot_inductor.output_path, so the files are in inductor's cache directory, which by default is under the system temp directory (torch's default, not checked here). If the output is on another file system, such as a tmpfs /tmp or a separate data disk, every buffer fails the st_dev check at _cord.py:160 and is copied.
  • When nothing can move (that case, Windows, unowned or shared data, or a program without file-backed data), _open_output still writes a temporary file, fsyncs it and renames it over path. That is an fsync a plain save doesn't do, and it gives the output a new inode. Other hard links keep the old content, and ownership, ACLs and extended attributes are not carried over.
  • Fix: decide the move set before opening the output; the checks need only a stat of each source and of path's directory. Warn once, naming each buffer that will be copied and why, and take the temporary-file path only when something will move. That also keeps the temporary file off Windows, so getattr(os, "O_BINARY", 0) at 1917 goes away. Fix this →
• save(consume=True) skips an overridden write_to_file. The plain path calls self.write_to_file (_program.py:2211), and test_save_works_with_a_subclass_that_overrides_write_to_file checks that it does. The consuming path calls self._pte_data._write_to_file (2209) instead, so a subclass or mock of write_to_file stops applying once consume=True is passed. Route both paths through one overridable method or document the difference, and cover consume=True in the test. Fix this →

Testing

• (still open) test_consume_moves_backend_data_through_real_lowering checks the .ptd with assertIn(payload, f.read()) (exir/program/test/test_program.py:1590). That passes even if the payload is at the wrong offset. Its 16 KiB payload is also a single chunk, so the backward chunk order never runs through real serialization. Deserialize the .ptd with FlatTensorSerializer().deserialize and compare the "payload" entry, as the .pte branch does. Use a payload larger than _COPY_CHUNK_SIZE, or patch the chunk size down.
• test_consume_moves_backend_data_from_a_deep_copied_edge_program (1294-1299) only checks that the source was emptied, not that the .pte holds the payload.
• No test covers os.replace failing after a complete move, though the docstring (_program.py:2194-2196) promises that the temporary file is kept and named.

Fix this →

Performance

• Overwriting an existing output saves no disk space.
  • _open_output keeps the old file at path until os.replace (_program.py:1940). A re-export to the same path therefore peaks at the old file plus the new data plus one chunk, which is about two copies. A plain save peaks at the same amount, because its open(path, "wb") frees the old file at once. The consuming save is still several times slower, and the same holds for each .ptd.
  • The docstring's "the peak is about one copy of the data instead of two" (2186) is only true for a new path. The caveat at 2196 doesn't say that the old file cancels the saving.
  • Fix: either empty the old file before data starts moving, or qualify the peak sentence. Emptying it gives up "If it fails, path is unchanged", which a plain save doesn't offer either. Fix this →

Code Quality

• _open_output decides whether data moved from the sizes of the source files (_program.py:1942), not from what _write_to_file actually moved, though CLAUDE.md asks for explicit state. Pass the moved buffers back explicitly, for example through a list that _open_output yields and _write_to_file appends to. Fix this →
• The finalizer is detached only in the except at _cord.py:201-202, which runs after the first truncation. If a second Ctrl-C lands in that handler before detach(), the finalizer stays armed. It later deletes the part of the buffer that is still in the source. Detach before the first ftruncate, and create a new finalizer when the move completes. Fix this →
• (still open) On macOS, os.fsync (_cord.py:188) doesn't flush the drive's cache. The guarantee in the comment at line 186 needs fcntl.fcntl(fd, fcntl.F_FULLFSYNC) there. This only matters for a power loss during a move.

Specific Comments

• exir/_serialize/_cord.py:77-78 - Deleting twice is harmless, since _remove ignores OSError. The real problem with an owning copy is that whichever one is collected first deletes the file out from under the other.
• exir/_serialize/_cord.py:127,167 - _move_to and _move_into differ only by a preposition. Name the inner one for what it does, for example _move_chunks.
• exir/program/_program.py:1935 - os.fchmod(output.fileno(), mode) would set the mode on the file just created, instead of looking temp up by name again. Fix this →
• exir/program/_program.py:1946 - The note says "the output written so far" even when only os.replace failed and the temporary file is complete. In that case it should say that the file can be renamed onto path.
• exir/program/_program.py:2188-2190 - "by this program or by the lowered program it came from" leaves out deep copies. __deepcopy__ returns self, so an edge program deep-copied for an ETRecord also loses the data. That is the case at test_program.py:1564.
• exir/program/test/test_program.py:1564 - "either may be lowered" is inaccurate, because both are already lowered by to_edge_transform_and_lower. The comment means that either may go through to_executorch.

Recommendation

Request Changes. The retry finding blocks approval: a consuming write that fails or is interrupted can't be retried, even for data it never moved. The overwrite finding removes the disk saving for re-exports to the same path. The silent fallback is still open from the earlier reviews. The other findings are minor.

Review steps

• Load the /pr-review skill
• Read the diff (HEAD~2..HEAD, 6 files, +1156/−36; the shallow clone has no merge base with origin/main)
• Run three independent reviewers
• Run the tests: not done, because running Python needs approval in this job
• Merge duplicate findings
• Check each finding against the code
• Check which points from the earlier reviews were addressed
• Post the review