Skip to content

refactor(sft): migrate SFTConfig, SFTSaveState to BaseModel#2525

Open
NolenLiang wants to merge 3 commits into
mainfrom
nliang/typeddict-to-basemodel-sft
Open

refactor(sft): migrate SFTConfig, SFTSaveState to BaseModel#2525
NolenLiang wants to merge 3 commits into
mainfrom
nliang/typeddict-to-basemodel-sft

Conversation

@NolenLiang
Copy link
Copy Markdown
Contributor

@NolenLiang NolenLiang commented May 19, 2026

Convert 2 TypedDict classes to pydantic BaseModel with extra="allow":

  • SFTConfig: 9 required fields
  • SFTSaveState: 5 fields with defaults + val_loss Optional[float]

Update all dict-style access to attribute access in sft.py. Wrap model_construct sft dict in test_sft.py with SFTConfig.model_construct().

What does this PR do ?

Add a one line overview of what this PR aims to accomplish.

Issues

List issues that this PR closes (syntax):

Usage

  • You can potentially add a usage example below
# Add a code snippet demonstrating how to use this

Before your PR is "Ready for review"

Pre checks:

  • Make sure you read and followed Contributor guidelines
  • Did you write any new necessary tests?
  • Did you run the unit tests and functional tests locally? Visit our Testing Guide for how to run tests
  • Did you add or update any necessary documentation? Visit our Document Development Guide for how to write, build and test the docs.

Results before / after the changes

image

@NolenLiang NolenLiang requested review from a team as code owners May 19, 2026 02:13
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented May 19, 2026

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@NolenLiang NolenLiang added the CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version) label May 19, 2026
@NolenLiang
Copy link
Copy Markdown
Contributor Author

/ok to test 3238765

@NolenLiang
Copy link
Copy Markdown
Contributor Author

/ok to test 9fb9969

Convert 2 TypedDict classes to pydantic BaseModel with extra="allow":
- SFTConfig: 9 required fields
- SFTSaveState: 5 fields with defaults + val_loss Optional[float]

Update all dict-style access to attribute access in sft.py.
Wrap model_construct sft dict in test_sft.py with SFTConfig.model_construct().

Signed-off-by: nliang <nliang@nvidia.com>
Signed-off-by: nliang <nliang@nvidia.com>
@NolenLiang NolenLiang force-pushed the nliang/typeddict-to-basemodel-sft branch from 9fb9969 to 74d09f7 Compare May 19, 2026 06:04
@NolenLiang
Copy link
Copy Markdown
Contributor Author

/ok to test 74d09f7

Add defaults to SFTConfig fields matching the reference config (sft.yaml).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: nliang <nliang@nvidia.com>
@NolenLiang
Copy link
Copy Markdown
Contributor Author

/ok to test 6231300

@NolenLiang NolenLiang added CI:L1 Run doctests, unit tests, and functional tests and removed CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version) labels May 19, 2026
@NolenLiang
Copy link
Copy Markdown
Contributor Author

/ok to test 6231300

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI:L1 Run doctests, unit tests, and functional tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant