-
Notifications
You must be signed in to change notification settings - Fork 6.7k
[core] gracefully error out when attn-backend x cp combo isn't supported. #12832
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
base: main
Are you sure you want to change the base?
Conversation
|
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. |
| raise ValueError(f"`{backend=}` must be one of the following: " + ", ".join(available_backends)) | ||
|
|
||
| backend = AttentionBackendName(backend) | ||
| if not _AttentionBackendRegistry._is_context_parallel_available(backend): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will raise an error if the backend doesn't support context parallel (regardless of whether you want to use CP or not)
We need to first check if the attention processor has a _parallel_config set and then check if the backend supports context parallel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should be fixed now. Thanks for flagging.
What does this PR do?
We should be able to error out when an attention backend isn't supported with CP.
Refer to #12829 (comment) and #12829 (comment).
Additionally, when we specify
parallel_configtofrom_pretrained()and then set an attention backend throughset_attention_backend()(which is CP-incompatible), it goes through silently.This PR eliminates those and gracefully errors out.