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

Add attention_backend to let user choose #1456

Merged
merged 2 commits into from
Jan 22, 2025

Conversation

yanbing-j
Copy link
Contributor

This PR is submitted for #1452, to add an argument attention_backend to let user choose the backend used in SDPA.

For CPU, only math and flash_attention are valid. For CUDA, all 4 backends are valid.

Copy link

pytorch-bot bot commented Jan 13, 2025

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/torchchat/1456

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure, 2 Unrelated Failures

As of commit a9dfc0e with merge base 2fc98f7 (image):

NEW FAILURE - The following job has failed:

BROKEN TRUNK - The following jobs failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Jan 13, 2025
@yanbing-j
Copy link
Contributor Author

Copy link
Contributor

@lucylq lucylq 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 making these changes! This looks good to me.

cc @Jack-Khuu

@lucylq lucylq requested a review from Jack-Khuu January 13, 2025 22:39
@Jack-Khuu
Copy link
Contributor

Looks like the ET pin needs bumping so I'll spin that up

Comment on lines 1182 to 1185
if self.builder_args.device == "cpu" and (self.builder_args.attention_backend == "efficient_attention"
or self.builder_args.attention_backend == "cudnn_attention"):
print(f"Warning: {self.builder_args.attention_backend} is not supported on CPU. Using math instead.")
self.builder_args.attention_backend = "math"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we bump this into the constructor of BuilderArgs?

@@ -69,6 +69,7 @@ class BuilderArgs:
prefill_possible: bool = False
dynamic_shapes: bool = False
max_seq_length: Optional[int] = None
attention_backend: str = "math"
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding the change!! Mind bumping the casting to the actual kernel into the body here?

That way we can do any sanity checks early and aren't passing around a string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have updated. Please review again!

@Jack-Khuu
Copy link
Contributor

@angelayi I recall you mentioning that there might be export issue with the other SDP backend?

Do you know if this issue was addressed? Or should be gate the backends to non-export?

@Jack-Khuu
Copy link
Contributor

Seems like yall had a chance to explore the export issue in the tagged issue

Let's add a check in export.py that flags when someone tries to use not Math.SDPA, to avoid users accidentally stumbling into it

Comment on lines 539 to 544
sdp_backend_dict = {
'math': torch.nn.attention.SDPBackend.MATH,
'flash_attention': torch.nn.attention.SDPBackend.FLASH_ATTENTION,
'efficient_attention': torch.nn.attention.SDPBackend.EFFICIENT_ATTENTION,
'cudnn_attention': torch.nn.attention.SDPBackend.CUDNN_ATTENTION,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we push this into the builder.py as well?

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.

bump this into the constructor of BuilderArgs
@mikekgfb
Copy link
Contributor

mikekgfb commented Jan 18, 2025

@angelayi I recall you mentioning that there might be export issue with the other SDP backend?

Do you know if this issue was addressed? Or should be gate the backends to non-export?

FLASH seemed to work for AOTI. Maybe we can at least allow a subset, and then methodically put them all thru a new test patter -- which we should anyway, to test the new options for generate as well. (suggest as separate PR, not piggyback here...)

for backend in math flash cudnn efficient
do 
  # cuda
  py3 tc generate stories15M --attention-backend $backend --prompt "hello there"
  py3 tc generate stories15M --attention-backend $backend --prompt "hello there" --compile --compile-prefill
  py3 tc export stories15M --attention-backend $backend --output-dso s.dso
  py3 tc generate stories15M --attention-backend $backend --prompt "hello there" -dso s.dso
  # cpu
  py3 tc generate stories15M --attention-backend $backend --prompt "hello there" -device cpu
  py3 tc generate stories15M --attention-backend $backend --prompt "hello there" --compile --compile-prefill -device cpu
  py3 tc export stories15M --attention-backend $backend --output-dso s.dso -device cpu
  py3 tc generate stories15M --attention-backend $backend --prompt "hello there" -dso s.dso -device cpu
done

@Jack-Khuu Jack-Khuu merged commit 45cd239 into pytorch:main Jan 22, 2025
54 of 57 checks passed
@Jack-Khuu
Copy link
Contributor

Thanks again @yanbing-j, merging this in

We can work on creating a support matrix separately

@yanbing-j yanbing-j deleted the yanbing/fix_1452 branch January 22, 2025 01:57
vmpuri pushed a commit that referenced this pull request Feb 4, 2025
bump this into the constructor of BuilderArgs

Co-authored-by: Jack-Khuu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants