pytorch / torchtune

PyTorch native finetuning library
https://pytorch.org/torchtune/main/
BSD 3-Clause "New" or "Revised" License
4.35k stars 440 forks source link

Remove unused FSDP components #2016

Closed ebsmothers closed 5 days ago

ebsmothers commented 6 days ago

I was attempting to patch the changes from #1933 so we can merge them, but I'm unable to push to the remote branch (likely because the PR was opened off of the fork's main branch which has not yet been merged with latest changes from upstream). So I've just patched everything into this new PR instead.

Tagging @krammnic as the author of the original PR. The only changes I've made were merging latest changes from main and updating test_validate_missing_and_unexpected_for_lora to get it to pass (and adding some of the test cases from validate_state_dict_for_lora that were deleted)

pytorch-bot[bot] commented 6 days ago

:link: Helpful Links

:test_tube: See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/torchtune/2016

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

:heavy_exclamation_mark: 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

:white_check_mark: No Failures

As of commit f772fbd9386d013cff5a193897b6d9c0f9d6c08c with merge base ac14e96443e60fd8ffed434637357ce22a9c5bff (image): :green_heart: Looks good so far! There are no failures yet. :green_heart:

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

codecov-commenter commented 6 days ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 64.47%. Comparing base (f15ba77) to head (ba9c60e). Report is 2 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2016 +/- ## =========================================== + Coverage 24.74% 64.47% +39.72% =========================================== Files 317 317 Lines 17669 17535 -134 =========================================== + Hits 4373 11305 +6932 + Misses 13296 6230 -7066 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

SalmanMohammadi commented 6 days ago

One small thing - is it worth deprecating the public APIs and deleting next release? I'm not sure if people are really using these so your call.

SalmanMohammadi commented 6 days ago

Some big ol nits due to some floating references - thanks for picking this up : )

ebsmothers commented 6 days ago

One small thing - is it worth deprecating the public APIs and deleting next release? I'm not sure if people are really using these so your call

Yeah this is a fair question. In this case I am gonna yolo a bit because we haven't supported FSDP1 for multiple releases at this point. And I think most of these APIs should not really have been public to begin with, but we made them so by necessity because many were used directly in our recipes