-
Notifications
You must be signed in to change notification settings - Fork 517
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
[export] Add exportable attention and kv cache #2049
Conversation
Summary: As titled. Test Plan: Rely on new unit tests Reviewers: Subscribers: Tasks: Tags:
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/torchtune/2049
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 6e910f1 with merge base e9fd56a (): This comment was automatically generated by Dr. CI and updates every 15 minutes. |
.github/workflows/unit_test.yaml
Outdated
NIGHTLY_VERSION=dev20241121 | ||
python -m pip install torch=="2.6.0.${NIGHTLY_VERSION}" torchvision=="0.20.0.${NIGHTLY_VERSION}" --extra-index-url https://download.pytorch.org/whl/nightly/cpu | ||
python -m pip install torchao |
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.
I don't think we want to change the default behavior of our unit test workflow here. Can we figure out a way to decouple the export CI a little more?
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.
Can you say a bit more? Are you ok with a separate CI job running these tests with torch nightly installed?
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.
Added Export
CI workflow.
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
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.
Thanks for putting up this first commit. I'm not reviewing the modules themselves but the integration with unit tests as this is the first time we're doing this. I left some comments related to this.
OK just discussed with @pbontrager. Let's do the following:
The workflow should run only on tests in the
|
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
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.
Should we also copy over the README?
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2049 +/- ##
==========================================
+ Coverage 67.69% 68.22% +0.53%
==========================================
Files 323 323
Lines 17823 17961 +138
==========================================
+ Hits 12065 12254 +189
+ Misses 5758 5707 -51 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Summary: As titled.
Test Plan: Rely on new unit tests
Reviewers:
Subscribers:
Tasks:
Tags:
Context
What is the purpose of this PR? Is it to
Please link to any issues this PR addresses.
Changelog
What are the changes made in this PR?
attention.py
intorchtune/modules/_export/
kv_cache.py
intorchtune/modules/_export/
test_attention.py
intests/torchtune/modules/_export/
Test plan
Please make sure to do each of the following if applicable to your PR. If you're unsure about any one of these just ask and we will happily help. We also have a contributing page for some guidance on contributing.
pre-commit install
)pytest tests
pytest tests -m integration_test
UX
If your function changed a public API, please add a dummy example of what the user experience will look like when calling it.
Here is a docstring example
and a tutorial example