Skip to content

feat: add ignored_params support for fsdp2 #3731

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Aug 18, 2025

Conversation

kmehant
Copy link
Contributor

@kmehant kmehant commented Aug 13, 2025

What does this PR do?

This allows reusing ignored_modules user facing FSDP API for FSDP v2 to set ignored_params.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@SunMarc @zach-huggingface @S1ro1

Signed-off-by: Mehant Kammakomati <mehant.kammakomati2@ibm.com>
Signed-off-by: Mehant Kammakomati <mehant.kammakomati2@ibm.com>
Signed-off-by: Mehant Kammakomati <mehant.kammakomati2@ibm.com>
Signed-off-by: Mehant Kammakomati <mehant.kammakomati2@ibm.com>
@S1ro1
Copy link
Member

S1ro1 commented Aug 13, 2025

Hello, thank you for the PR. Can you also include it in the tests?

@kmehant
Copy link
Contributor Author

kmehant commented Aug 13, 2025

Hi 👋🏻 @S1ro1

Yes I will update the PR with the test.

Signed-off-by: Mehant Kammakomati <mehant.kammakomati2@ibm.com>
@kmehant
Copy link
Contributor Author

kmehant commented Aug 13, 2025

@S1ro1 Have added it.
Screenshot 2025-08-13 at 7 50 45 PM

@kmehant
Copy link
Contributor Author

kmehant commented Aug 14, 2025

@S1ro1 Should I bump up the torch version FSDP2_PYTORCH_VERSION to 2.7.0 from 2.6.0? Since ignored_params is available from 2.7.0? Or you want me to handle it separately?

@kmehant
Copy link
Contributor Author

kmehant commented Aug 18, 2025

@S1ro1 appreciate any update ?

@S1ro1
Copy link
Member

S1ro1 commented Aug 18, 2025

@kmehant Sorry, missed the update. I think it should be handled separately, something in post_init of fsdp plugin, along the lines of if version < 2.7.0 and ignored_params not None and fsdp_version == 2

Signed-off-by: Mehant Kammakomati <mehant.kammakomati2@ibm.com>
@kmehant
Copy link
Contributor Author

kmehant commented Aug 18, 2025

@S1ro1 thank you for the response, does the current change sound good? Also, do you think we should expose ignored_params as an option for the user to provide apart from having ignored_modules as a proxy?

Copy link
Member

@S1ro1 S1ro1 left a comment

Choose a reason for hiding this comment

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

LGTM

@S1ro1
Copy link
Member

S1ro1 commented Aug 18, 2025

@kmehant I think let's see if this gets some usage, can you think of some common use-cases where you'd like to use ignored_params directly? I feel like ignored_params/modules aren't that commonly used anyway.

Signed-off-by: Mehant Kammakomati <mehant.kammakomati2@ibm.com>
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@kmehant
Copy link
Contributor Author

kmehant commented Aug 18, 2025

@S1ro1

I think let's see if this gets some usage, can you think of some common use-cases where you'd like to use ignored_params directly?

I don't have a usecase for ignored_params we can keep it for future.

I feel like ignored_params/modules aren't that commonly used anyway.

We have a specific use case for this, this PR will be helpful.

@kmehant
Copy link
Contributor Author

kmehant commented Aug 18, 2025

Failed test seems unrelated

@S1ro1 S1ro1 merged commit a7d6f28 into huggingface:main Aug 18, 2025
24 of 25 checks passed
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.

3 participants