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

[tests] enable test_mixed_adapter_batches_lora_opt_timing on XPU #2021

Merged
merged 4 commits into from
Aug 21, 2024

Conversation

faaany
Copy link
Contributor

@faaany faaany commented Aug 20, 2024

After Fix:

====================================== short test summary info ======================================
PASSED tests/test_custom_models.py::TestMixedAdapterBatches::test_mixed_adapter_batches_lora_opt_timing
=================================== 1 passed, 1 warning in 7.59s ====================================

Just like other tests in this file, this function should not only apply to NV GPU. We can actually remove this test marker.

@faaany
Copy link
Contributor Author

faaany commented Aug 20, 2024

@BenjaminBossan

Copy link
Member

@BenjaminBossan BenjaminBossan left a comment

Choose a reason for hiding this comment

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

The issue with this change is that this would mean the test also runs on CPU. As the comment further below indicates, we want to avoid this to prevent flakiness:

# Measure timing of running base and adapter separately vs using a mixed batch. Note that on CPU, the
# differences are quite small, so this test requires GPU to avoid flakiness.

I tried the test again just now on CPU and the time differences are indeed much smaller (~25%) compared to GPU (~150%), so this is still true. One solution would be to check if either GPU or XPU is being used.

@faaany
Copy link
Contributor Author

faaany commented Aug 20, 2024

The issue with this change is that this would mean the test also runs on CPU. As the comment further below indicates, we want to avoid this to prevent flakiness:

# Measure timing of running base and adapter separately vs using a mixed batch. Note that on CPU, the
# differences are quite small, so this test requires GPU to avoid flakiness.

I tried the test again just now on CPU and the time differences are indeed much smaller (~25%) compared to GPU (~150%), so this is still true. One solution would be to check if either GPU or XPU is being used.

Sure, should I add a marker called "require_torch_accelerator" just like in accelerate?

@BenjaminBossan
Copy link
Member

Sure, should I add a marker called "require_torch_accelerator" just like in accelerate?

Hmm, I think let's not go that far yet, a simple check at the start of the test function for the device is more explicit, we can add an extra decorator if we need the same check in many places.

@faaany
Copy link
Contributor Author

faaany commented Aug 20, 2024

Sure, should I add a marker called "require_torch_accelerator" just like in accelerate?

Hmm, I think let's not go that far yet, a simple check at the start of the test function for the device is more explicit, we can add an extra decorator if we need the same check in many places.

Sure, let me update!

@faaany
Copy link
Contributor Author

faaany commented Aug 21, 2024

@BenjaminBossan how about the require_non_cpu solution I introduced in this PR #2026 ?

@BenjaminBossan
Copy link
Member

how about the require_non_cpu solution I introduced in this PR #2026 ?

Sounds good, I merged that PR.

@faaany
Copy link
Contributor Author

faaany commented Aug 21, 2024

rebase done.

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

@BenjaminBossan BenjaminBossan 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 extending this test, LGTM.

Btw. is there some public place where I can check the XPU tests?

@BenjaminBossan BenjaminBossan merged commit fa218e1 into huggingface:main Aug 21, 2024
14 checks passed
@faaany
Copy link
Contributor Author

faaany commented Aug 22, 2024

Thanks for extending this test, LGTM.

Btw. is there some public place where I can check the XPU tests?

do you mean the test summary on XPU?

@BenjaminBossan
Copy link
Member

do you mean the test summary on XPU?

Yes, so that me and others can check the current state.

@faaany
Copy link
Contributor Author

faaany commented Aug 23, 2024

do you mean the test summary on XPU?

Yes, so that me and others can check the current state.

Sure, currently we don't upload the test results to a public repo. But let me come out with a solution. Talk to you next Monday.

@BenjaminBossan
Copy link
Member

Sure, currently we don't upload the test results to a public repo. But let me come out with a solution. Talk to you next Monday.

Thanks. It's not super high priority, but right now if we ever break something in PEFT for XPU, we won't know until someone comes to us to report it.

@yao-matrix
Copy link

Sure, currently we don't upload the test results to a public repo. But let me come out with a solution. Talk to you next Monday.

Thanks. It's not super high priority, but right now if we ever break something in PEFT for XPU, we won't know until someone comes to us to report it.

We have a 2-step plan:

  1. screen HF libraries(transformers, accelerate, peft, diffusers, trl) UTs, run in XPU, and fix or extend if we target them run on XPU
  2. work w/ Hugging Face to provide CI machines and integrate into CI flow
    We are in 1st now, and progress is transformers & accelerate & peft are done, diffusers & trl are undergoing. Once this step is done, we are glad to work w/ you guys to enable CI, :).

@BenjaminBossan
Copy link
Member

Thanks for the update on the plan.

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