Skip to content

Load an older PTD file in the Python reader instead of rejecting it - #22117

Merged
shoumikhin merged 1 commit into
mainfrom
py-flat-tensor-version-le
Aug 25, 2026
Merged

Load an older PTD file in the Python reader instead of rejecting it#22117
shoumikhin merged 1 commit into
mainfrom
py-flat-tensor-version-le

Conversation

@shoumikhin

Copy link
Copy Markdown
Contributor

Summary

The Python FlatTensorSerializer.deserialize refused any PTD file whose version did not exactly equal _FLAT_TENSOR_VERSION:

if flat_tensor.version != _FLAT_TENSOR_VERSION:
    raise NotImplementedError(...)

That rejects an older file too, which contradicts:

  • the append-only schema policy in schema/README.md (older files stay loadable), and
  • the C++ runtime readers, which accept anything <= their supported version and only refuse a file newer than they understand (Program::load, FlatTensorDataMap::load).

This aligns the Python PTD reader with that policy: compare with > instead of !=, so an older or equal file loads and only a newer one is refused. The error message now says the file is newer than this reader supports.

Context

This was called out as a follow-up during review of #22114 (which adds the <= gate on the C++ side). It keeps the Python export/tooling path consistent so a future non-zero PTD version won't regress it.

Test plan

Two tests in extension/flat_tensor/test/test_serialize.py:

  • test_deserialize_refuses_newer_version — a bumped-version file is refused.
  • test_deserialize_accepts_older_version — a file older than the reader still loads (the case the old != wrongly rejected).

Both are behavior-only; no schema or format change. Version constants are unchanged (still 0), so this is inert for existing files and only changes behavior once a non-zero version is ever stamped.

Copilot AI lite review requested due to automatic review settings August 25, 2026 01:29
@pytorch-bot

pytorch-bot Bot commented Aug 25, 2026

Copy link
Copy Markdown

🔗 Helpful Links

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

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

✅ No Failures

As of commit b7ae07a with merge base 1afd07f (image):
💚 Looks good so far! There are no failures yet. 💚

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 25, 2026
@github-actions

Copy link
Copy Markdown

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

@shoumikhin

Copy link
Copy Markdown
Contributor Author

@claude review the PR

@claude

claude Bot commented Aug 25, 2026

Copy link
Copy Markdown

Claude finished @shoumikhin's task in 49s —— View job


PR Review

  • Read PR diff
  • Analyze serialize.py change
  • Analyze tests
  • Post review

Verdict: LGTM. The change is small, correct, and well-motivated. It brings the Python reader in line with the append-only schema policy and the C++ runtime's "reject only newer" behavior.

Correctness

  • The !=> swap is the right fix. With _FLAT_TENSOR_VERSION = 0 today, the guard is inert for existing files (a file can't have a negative version on a uint32 schema field), so this is a safe no-op until a non-zero version is ever stamped — exactly as the description claims.
  • Both tests exercise the right directions and are self-consistent:
    • test_deserialize_refuses_newer_version: serialize under a patched +1 version, deserialize with the real reader → 1 > 0 → refused. ✅
    • test_deserialize_accepts_older_version: serialize at current version, deserialize with the reader patched to +10 > 1 is false → loads. ✅ This is the case the old != wrongly rejected.
  • The updated error message is clearer and now correctly distinguishes "newer than supported" from a generic mismatch. Minor grammar nit fixed too (old text read "files reports").

Minor suggestions (non-blocking)

  1. test_deserialize_accepts_older_version doesn't assert the result. It relies on "must not raise." Consider asserting the payload round-trips (e.g. named_data keys match TEST_DATA_PAYLOAD), like the existing test_serialize_deserialize does — otherwise a future regression that loads but drops data would pass silently. Fix this →

  2. from unittest import mock inside each test. Small style thing — could hoist to a module-level import to match the file's other top-level imports. Not worth churn if you prefer keeping it local.

  3. Consistency check — the PR description says this matches the C++ <= gate added in Refuse a PTE or PTD file whose schema version is newer than the runtime can read #22114. Worth confirming that gate has landed (or lands together) so the stated alignment actually holds; the Python side here is safe regardless.

Nothing blocking. Nice cleanup.
· branch py-flat-tensor-version-le

FlatTensorSerializer.deserialize refused any file whose version did not exactly
equal _FLAT_TENSOR_VERSION. That rejects an older file too, which contradicts the
append-only schema policy (schema/README.md) and the C++ runtime readers, which
accept anything at or below their supported version and only refuse a file newer
than they understand.

Compare with > instead of !=, so an older or equal file loads and only a newer
one is refused, and reword the error to say the file is newer than this reader
supports. Two tests cover both directions: a bumped-version file is refused, and
a file older than the reader still loads (the case the old != wrongly rejected).
@shoumikhin
shoumikhin force-pushed the py-flat-tensor-version-le branch from b7ae07a to 26cf683 Compare August 25, 2026 02:11
Copilot AI review requested due to automatic review settings August 25, 2026 02:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@shoumikhin

Copy link
Copy Markdown
Contributor Author

Thanks @claude. Addressed both actionable notes:

  1. Round-trip assertiontest_deserialize_accepts_older_version now checks the deserialized payload matches the original (buffers + named_data), reusing the same helper as the main round-trip test, so a "loads but drops data" regression can't pass silently.
  2. mock import — hoisted to a module-level from unittest import mock.

On #3: yes, the C++ <= gate is #22114 (the companion PR). The Python side here is safe either way, as you noted.

@claude

claude Bot commented Aug 25, 2026

Copy link
Copy Markdown

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


Follow-up Review

  • Read updated serialize.py
  • Verify round-trip assertion in test_deserialize_accepts_older_version
  • Verify mock import hoisted to module level

Verdict: LGTM. Both notes are addressed cleanly and the change is ready.

Round-trip assertion ✅

test_deserialize_accepts_older_version now compares buffers element-wise and reuses _check_named_data_entries — the same helper the main round-trip test (test_serialize_deserialize) uses. A "loads but drops data" regression can no longer pass silently, exactly the gap called out. The comment explaining why the assertion exists is a nice touch.

mock import ✅

Hoisted to a module-level from unittest import mock, matching the file's other top-level imports. Both new tests reference it without local re-imports.

Everything else still holds

  • The !=> swap remains correct: older/equal files load, only a strictly-newer file is refused — consistent with the C++ <= gate in Refuse a PTE or PTD file whose schema version is newer than the runtime can read #22114 and the append-only schema policy.
  • Both tests patch _FLAT_TENSOR_VERSION in the right direction: refuses_newer serializes at +1 and reads with the real reader (1 > 0 → refused); accepts_older serializes at current and reads with the reader bumped to +1 (0 > 1 false → loads and round-trips).
  • Inert for existing files while the version stays 0.

Nothing blocking — good to merge.
· branch py-flat-tensor-version-le

@shoumikhin
shoumikhin merged commit 6a315a4 into main Aug 25, 2026
193 of 194 checks passed
@shoumikhin
shoumikhin deleted the py-flat-tensor-version-le branch August 25, 2026 02:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants