Skip to content

fix(storage): preserve allowed Pydantic extras in snapshots 🤖🤖🤖 - #280

Open
Sampoornnagpal wants to merge 2 commits into
NVIDIA-NeMo:mainfrom
Sampoornnagpal:fix/snapshot-pydantic-extras
Open

fix(storage): preserve allowed Pydantic extras in snapshots 🤖🤖🤖#280
Sampoornnagpal wants to merge 2 commits into
NVIDIA-NeMo:mainfrom
Sampoornnagpal:fix/snapshot-pydantic-extras

Conversation

@Sampoornnagpal

@Sampoornnagpal Sampoornnagpal commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Snapshots silently drop allowed Pydantic extras. For example, a ModelConfig with num_retries=7 restores without that supported provider setting. Serialize model_extra values recursively and retain them during restoration when the model declares extra="allow".

Nested extras use the existing typed envelopes, class allowlist, and nosnapshot handling. Models that ignore or forbid extras keep the existing lenient restoration behavior for schema drift.

Validation

The four new cases fail before the fix. uv run pytest -q tests/storage — 101 passed, including existing strict-model and allowlist tests.

Windows/Python 3.12 with external import-only helpers for the existing fcntl/SIGUSR2 blockers (#84/#85). Helpers are not included and do not validate locks/signals. No live provider calls.

Related issues

No matching open issue found.

Checklist

  • Ruff lint and formatting pass.
  • Regression and storage tests pass.
  • Existing snapshot format and strict-model behavior retained.
  • Existing SPDX headers retained.

Summary by CodeRabbit

  • Bug Fixes
    • Preserved additional provider fields during serialization and deserialization when models allow extra data.
    • Continued filtering unknown fields for models that do not permit extras.
    • Improved handling of nested dataclass and snapshotable values during serialization.
    • Prevented values marked for exclusion from appearing in serialized output.
    • Preserved ordinary declared fields through serialization and deserialization round trips.

Signed-off-by: Sampoorn Nagpal <nagpalsampoorn@gmail.com>
@coderabbitai

coderabbitai Bot commented Sep 5, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 45586e02-146b-4ccc-8869-68d8776ad5e4

📥 Commits

Reviewing files that changed from the base of the PR and between 732f635 and 575ec6c.

📒 Files selected for processing (1)
  • tests/storage/test_serialization.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/storage/test_serialization.py

Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

Pydantic serialization now includes eligible model_extra fields. Deserialization restores extras for models configured with extra="allow". Tests cover nested values, filtering, disallowed classes, and __nosnapshot__ fields.

Changes

Serialization extras

Layer / File(s) Summary
Extra-field serialization and restoration
src/nooa/storage/serialization.py, tests/storage/test_serialization.py
Serialization includes non-SKIP model_extra values. Deserialization retains extras for extra="allow" models and continues filtering unknown fields for other model policies. Tests cover recursive restoration, allowlist enforcement, ordinary field roundtrips, and __nosnapshot__ omission.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to 575ec

Snapshot restoration now preserves allowed Pydantic extra fields while retaining filtering and lenient schema-drift behavior. No merge-blocking risk is currently identified.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the storage fix: preserving allowed Pydantic extras in snapshots. The emojis add minor noise but do not obscure the main change.
Docstring Coverage ✅ Passed Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

Signed-off-by: Sampoorn Nagpal <nagpalsampoorn@gmail.com>
@alessiodevoto alessiodevoto self-assigned this Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants