SFT dataset - model transform validation

[andrewldesousa]

Context

What is the purpose of this PR? Is it to

• add a new feature
• fix a bug
• update tests and/or documentation
• other (improve engineering practices and raising value errors for bad transforms)

Please link to any issues this PR addresses.
#1326

Changelog

What are the changes made in this PR?
Raise value error and unit tests for this code path

Test plan

Please make sure to do each of the following if applicable to your PR. (If you're not sure about any one of these just ask and we will happily help. We also have a contributing page for some guidance on contributing.)

• run pre-commit hooks and linters (make sure you've first installed via pre-commit install)
• add unit tests for any new functionality
• update docstrings for any new or updated methods or classes
• run unit tests via pytest tests
• run recipe tests via pytest tests -m integration_test
• manually run any new or modified recipes with sufficient proof of correctness
• include relevant commands and any other artifacts in this summary (pastes of loss curves, eval results, etc.)

UX

If your function changed a public API, please add a dummy example of what the user experience will look like when calling it.
Example of docstring:

torchtune/torchtune/modules/vision_transformer.py

Line 285 in 6a7951f

Examples:

Example in our docs: https://pytorch.org/torchtune/main/tutorials/qat_finetune.html#applying-qat-to-llama3-models

• I did not change any public API;
• I have added an example to docs or docstrings;

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/torchtune/1470

• 📄 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 a20e23a with merge base 0b9f830 ():
💚 Looks good so far! There are no failures yet. 💚

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

[andrewldesousa]

cc: @joecummings @RdoubleA

[felipemello1]

hey @andrewldesousa , thanks for the PR! A few questions:

1. Is this the only dataset that needs it?

2. The if condition may break if the output is not an iterable (you are checking if "key" in X). Should we first confirm that the output is a dictionary?

3. In the error message, to help with debugging, should we output the keys found?

[andrewldesousa]

hey @andrewldesousa , thanks for the PR! A few questions:

1. Is this the only dataset that needs it?
2. The if condition may break if the output is not an iterable (you are checking if "key" in X). Should we first confirm that the output is a dictionary?
3. In the error message, to help with debugging, should we output the keys found?

1. For validating model transform, yes. There is a potential opportunity to validate the same keys returned by PackedDataset if that is valuable to do, but i think that should be considered a separate issue if it is even an issue at all.
2. I think either approach here is viable, but I think that 1) the same issue occurs in the message_transform call above and through various parts of the code base. it might add more friction to developers to here to type check every similar situation but of course this is open to discussion. 2) its fine to rely on users here to use the SFTDataset correctly and implement valid transforms which adhere to the Transform spec given there is fairly clear documentation for what is expected of the user. 3) the datasets in this repos that will leverage this class are should be protected against this issue from code review and testing

[felipemello1]

lgtm

[RdoubleA]

Thanks for covering this and adding a test. Just need to fix lint and we can merge