Skip to content

Conversation

@jojivk73
Copy link

@jojivk73 jojivk73 commented Feb 12, 2025

What does this PR do?

Fixes # (issue)
The version checks for pytorch when psdp plugin is used has a format issue. This causes an error when fsdp plugins are enabled. The fix is in line with the original accelerate code.

https://github.com/huggingface/accelerate/blob/526925b48c07d997cdd9bf5911f659ca45778915/src/accelerate/accelerator.py#L364

Before submitting

  • [Y] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • [N] Did you make sure to update the documentation with your changes?
    fsdp plugins fail
  • [N] Did you write any new necessary tests?

The version checks for pytorch when psdp plugin is used has a format issue. This causes an error when fsdp plugins are enabled. The fix is in line with the original accelerate code.

https://github.com/huggingface/accelerate/blob/526925b48c07d997cdd9bf5911f659ca45778915/src/accelerate/accelerator.py#L364
@jojivk73 jojivk73 requested a review from regisss as a code owner February 12, 2025 23:19
@emascarenhas
Copy link
Contributor

Please run through the tests for fsdp - RUN_SLOW=true GAUDI2_CI=1 pytest tests/test_fsdp_examples.py -v -s
and report summary results from the tests here.

import importlib.metadata

torch_version = importlib.metadata.version("torch")
torch_version = torch_version[5:]
Copy link
Contributor

Choose a reason for hiding this comment

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

if we don't need torch_version anymore, these lines setting torch_version could be deleted?

@emascarenhas
Copy link
Contributor

@vivekgoe , Could you do a quick review of this?

@vivekgoe
Copy link
Collaborator

@emascarenhas Since check ensures that we are running pytorch version >= 2.1.0 which is quite old, therefore in my opinion it should be ok to remove the check completely. If we go down this path of removing FSDP version check, please also remove it from other places in optimum-habana (e.g. optimum-habana-fork/optimum/habana/accelerate/utils/other.py).
Please also update examples/language-modeling/README.md FSDP example with disclaimer that feature works only for pytorch version >= 2.1.0 to be safe.

@emascarenhas
Copy link
Contributor

@emascarenhas Since check ensures that we are running pytorch version >= 2.1.0 which is quite old, therefore in my opinion it should be ok to remove the check completely. If we go down this path of removing FSDP version check, please also remove it from other places in optimum-habana (e.g. optimum-habana-fork/optimum/habana/accelerate/utils/other.py). Please also update examples/language-modeling/README.md FSDP example with disclaimer that feature works only for pytorch version >= 2.1.0 to be safe.

@jojivk73 , Per @vivekgoe suggestions, please review the PR for additional changes to be consistent for FSDP across OH. I think it's ok to retain the check as we do not want an older pytorch to be used or in the future maybe we enforce a newer version of pytorch.

@emascarenhas
Copy link
Contributor

Looks good.
@jojivk73 , Are the test_fsdp_examples.py passing now? I think they are supposed to pass. @libinta , Is that right that tests should pass? Please add run_test to this PR once @jojivk73 confirms.

@emascarenhas
Copy link
Contributor

@jojivk73 , Any update here? If not needed anymore we can close it?

@jojivk73
Copy link
Author

@emascarenhas, This is still needed to use 1.19 with FSDP plugin. I will get back on this soon.

@emascarenhas
Copy link
Contributor

@emascarenhas, This is still needed to use 1.19 with FSDP plugin. I will get back on this soon.

@jojivk73 , Any update here? Please put in "Draft" state if not ready.

@jojivk73 jojivk73 marked this pull request as draft March 27, 2025 16:27
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.

4 participants