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 backend check #2670

Merged
merged 6 commits into from
Apr 17, 2024
Merged

fix backend check #2670

merged 6 commits into from
Apr 17, 2024

Conversation

jiqing-feng
Copy link
Contributor

@jiqing-feng jiqing-feng commented Apr 15, 2024

Hi @muellerzr
Related to #2652 . I think we'd better have a test on it.

I think the best way is to have an end-to-end test in the CI; it will avoid most issues : )

@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.

Copy link
Collaborator

@muellerzr muellerzr left a comment

Choose a reason for hiding this comment

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

Thanks! This looks a bit better on paper at least, I'll pull it down and test locally before giving the ✅

Left a few logic and style nits

src/accelerate/state.py Outdated Show resolved Hide resolved
src/accelerate/state.py Outdated Show resolved Hide resolved
original_backend = kwargs.pop("backend", None)
backend, distributed_type = self._prepare_backend(cpu, use_sagemaker_dp, original_backend)
if original_backend and backend != original_backend:
logger.warning(f"The assigned backend is {original_backend}, but the real backend is {backend}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it makes sense to do a warning here. This should be a raise if this is truly a situation we can get into.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see this change propagated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I forgot to push. It should be updated now, pls check. Thx.

jiqing-feng and others added 4 commits April 15, 2024 12:49
Co-authored-by: Zach Mueller <muellerzr@gmail.com>
Co-authored-by: Zach Mueller <muellerzr@gmail.com>
Copy link
Collaborator

@muellerzr muellerzr left a comment

Choose a reason for hiding this comment

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

Thanks so much! One last, last thing then we can merge and get a patch out ✅

src/accelerate/state.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@muellerzr muellerzr left a comment

Choose a reason for hiding this comment

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

Thanks for fixing!

@muellerzr muellerzr merged commit fd0dcd1 into huggingface:main Apr 17, 2024
23 checks passed
jiqing-feng and others added 2 commits April 17, 2024 04:52
Co-authored-by: Zach Mueller <muellerzr@gmail.com>
muellerzr added a commit that referenced this pull request Apr 17, 2024
* fix backend check

* reformat backend check

* Update src/accelerate/state.py

Co-authored-by: Zach Mueller <muellerzr@gmail.com>

* Update src/accelerate/state.py

Co-authored-by: Zach Mueller <muellerzr@gmail.com>

* raise value error if backend mismatch

* Update src/accelerate/state.py

Co-authored-by: Zach Mueller <muellerzr@gmail.com>

---------

Co-authored-by: Zach Mueller <muellerzr@gmail.com>
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