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 #2652

Merged
merged 2 commits into from
Apr 15, 2024
Merged

fix backend check #2652

merged 2 commits into from
Apr 15, 2024

Conversation

jiqing-feng
Copy link
Contributor

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

Hi @muellerzr . This PR fixed the backend assignment. It avoids confusing backend checks and distributed_type checks. We should assign distributed_type by env first and then decide whether to assign backend.

Do you know how to add a proper test to avoid this issue? Please let me know, and I will add it : )

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.

Sure, I'm open to this, so long as the tests pass. The tests specifically are all of the tests, which are run on XPU and multi-CPU hardware.

A simple check is accelerate test if you have the hardware available. If not let me know

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

@jiqing-feng
Copy link
Contributor Author

Sure, I'm open to this, so long as the tests pass. The tests specifically are all of the tests, which are run on XPU and multi-CPU hardware.

A simple check is accelerate test if you have the hardware available. If not, let me know

Hi @muellerzr, The accelerate test runs well. We can merge this PR to fix the problem as soon as possible, but we'd better design a test in CI to check that the configs are correctly assigned considering the long term.

@muellerzr muellerzr merged commit 2fc48c7 into huggingface:main Apr 15, 2024
23 checks passed
@zhangsheng377
Copy link
Contributor

@muellerzr
This modification changes all MULTI_GPU and MULTI_NPU to MULTI_CPU, right?

@muellerzr
Copy link
Collaborator

Yes it does, I didn’t have a chance to run it before merging on the CI, I can see many failures. Reverting

muellerzr added a commit that referenced this pull request Apr 15, 2024
muellerzr added a commit that referenced this pull request Apr 15, 2024
@muellerzr
Copy link
Collaborator

I’ll look at trying to do an alternative today

@jiqing-feng
Copy link
Contributor Author

I know why it failed.... The best way is to return in each branch. I will update a another PR to fix it quickly. Sorry for the inconvinient.

@muellerzr
Copy link
Collaborator

No worries @jiqing-feng :) Lots of moving parts.

I'll make sure to test it locally next time as well to make sure non-multi-cpu's don't break

@jiqing-feng jiqing-feng mentioned this pull request Apr 15, 2024
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