Skip to content
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

fix deepspeed available detection #26252

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

fxmarty
Copy link
Contributor

@fxmarty fxmarty commented Sep 19, 2023

As per title, make the deepspeed available function more robust as https://github.com/huggingface/accelerate/blob/69e4c3c54da3201eda288b500d138761e7a5221c/src/accelerate/utils/imports.py#L72

Having tests/deepspeed & get_env that adds tests/ in the path then makes is_deepspeed_availebl() returns True although it should not, and in turn trainer.py tries to import DeepSpeedSchedulerWrapper that is not imported in accelerate as accelerate rightfully detects that DeepSpeed is not available.

This issue makes the test tests/extended/test_trainer_ext.py::TestTrainerExt::test_run_seq2seq_apex fail when APEX is installed but DeepSpeed is not.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Sep 19, 2023

The documentation is not available anymore as the PR was closed or merged.

@ydshieh
Copy link
Collaborator

ydshieh commented Sep 19, 2023

Could you show us how to see the following?

Having tests/deepspeed & get_env that adds tests/ in the path then makes is_deepspeed_availebl() returns True

@fxmarty
Copy link
Contributor Author

fxmarty commented Sep 19, 2023

@ydshieh Reproduction (maybe add relevant breakpoint() and print sys.path as well):

docker run --rm -it --gpus all nvcr.io/nvidia/pytorch:23.08-py3 /bin/bash
pip list | grep apex  # apex is here!
git clone https://github.com/huggingface/transformers.git
cd transformers
pip install -e .[dev-torch]
pytest tests/extended/test_trainer_ext.py::TestTrainerExt::test_run_seq2seq_apex -s -vvvvv

@ydshieh
Copy link
Collaborator

ydshieh commented Sep 19, 2023

I tried to do it in a simple way

import os
# given by `get_env`
os.environ["PYTHONPATH"] = "/transformers/src:/transformers/tests:"
# shows `/transformers/src:/transformers/tests:`
print(os.environ["PYTHONPATH"])
from transformers.deepspeed import is_deepspeed_available
print(is_deepspeed_available())

and it prints False. Therefore, I am not very certain about the issue. Even with tests/, still False

@fxmarty
Copy link
Contributor Author

fxmarty commented Sep 19, 2023

@ydshieh You would need to use: PYTHONPATH=/path/to/transformers/tests python -c "from transformers.deepspeed import is_deepspeed_available; print(is_deepspeed_available())" to reproduce the issue.

I believe setting PYTHONPATH within a python script does not change sys.path (or somthing like this).

Copy link
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

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

Thank you @fxmarty . Nice fix.

@ydshieh
Copy link
Collaborator

ydshieh commented Sep 19, 2023

For the record, to reproduce (inside our docker env)

PYTHONPATH=/transformers/tests python3 -c "from transformers.deepspeed import is_deepspeed_available; print(is_deepspeed_available())"

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Good call, grateful for the detail pr description 😄

@ydshieh ydshieh merged commit 2455320 into huggingface:main Sep 20, 2023
parambharat pushed a commit to parambharat/transformers that referenced this pull request Sep 26, 2023
blbadger pushed a commit to blbadger/transformers that referenced this pull request Nov 8, 2023
EduardoPach pushed a commit to EduardoPach/transformers that referenced this pull request Nov 18, 2023
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