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.

Sign up for GitHub

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

Jump to bottom

[Blocksparse] bug fixing half + sequence length #25

Merged
merged 3 commits into from
Oct 22, 2021
Merged

[Blocksparse] bug fixing half + sequence length #25

merged 3 commits into from
Oct 22, 2021

Conversation

blefaudeux
Copy link
Contributor

@blefaudeux blefaudeux commented Oct 22, 2021 •
edited
Loading

What does this PR do?

Fixes #24.

  • blocksparse works on fp16
  • sequence length needs to be power of two for now
  • do not expose Blocksparse if the current GPU does not have tensor cores

Would be nice to follow up with a PR to Triton to fix the second point. cc @ptillet

Before submitting

  • Did you have fun?
    • Make sure you had fun coding 🙃
  • Did you read the contributor guideline?
  • Was this discussed/approved via a Github issue? (no need for typos, doc improvements)
    • N/A
  • Did you make sure to update the docs?
    • Doing this right now, updating the PR
  • Did you write any new necessary tests?
    • Sort of, we assert in the attention to catch this broken case and explain a little better
  • Did you update the changelog? (if needed)
    • N/A

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 22, 2021
blefaudeux
blefaudeux commented Oct 22, 2021
View reviewed changes
xformers/benchmarks/benchmark_encoder.py
@@ -325,12 +325,12 @@ def plot(args, results: List[Dict[str, Any]]):
"-emb", "--embedding_dim", nargs="+", default=[64, 128, 256], type=int
)
parser.add_argument(
"-sl", "--sequence_length", nargs="+", default=[512, 768, 1024], type=int
"-sl", "--sequence_length", nargs="+", default=[512, 1024], type=int
Copy link
Contributor Author

Choose a reason for hiding this comment

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

would have been nice to test for longer sequences, but 2048 OOMS with the vanilla attention on a V100..

@blefaudeux
Copy link
Contributor Author

Approved via internal chat :)

@blefaudeux blefaudeux merged commit 2e5906a into main Oct 22, 2021
@blefaudeux blefaudeux deleted the blocksparse_crash branch October 22, 2021 22:41
tenpercent added a commit to tenpercent/xformers that referenced this pull request Oct 8, 2024
@tenpercent
Merge pull request facebookresearch#25 from tenpercent/refactor-hip-d…
f37fb3d 
…ecoder

[Refactor] change ck decoder invocation way from old CK to CK-Tile
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jieru-hu jieru-hu Awaiting requested review from jieru-hu

@dianaml0 dianaml0 Awaiting requested review from dianaml0

@fmassa fmassa Awaiting requested review from fmassa

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Blocksparse crashes in the encoder benchmark
2 participants
@blefaudeux @facebook-github-bot