-
Notifications
You must be signed in to change notification settings - Fork 36
Archive RPAv2 and Delete Torchax-pt path. #504
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
Conversation
DescriptionStart with a short description of what the PR does and how this is a change from The rest of the description includes relevant details and context, examples:
If the change fixes a bug or a Github issue, please include a link, e.g.,: TestsPlease describe how you tested this change, and include any instructions and/or ChecklistBefore submitting this PR, please make sure:
|
|
Since we torchax is fixed.. Can we update the tests that were temporarily skipped? (maybe in a separated PR) |
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.
Any reason for deleting this file? I have a draft PR that relies on this code path: #512
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.
Because this file is for torchax-pt path which will not be used anymore.
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.
Since we torchax is fixed.. Can we update the tests that were temporarily skipped? (maybe in a separated PR)
hm... the tests are still failing with some type errors like
TypeError: Cannot interpret 'torch.bfloat16' as a data type
AttributeError: 'NoneType' object has no attribute 'dtype'
Should be able to be fixed.
It may take me some time...
I will address in anther PR to avoid making this one too big.
Is it ok?
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'm planning to use this file in my torchax-jax refactoring. Can you remove deletion of this file from this PR?
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.
hm.. why?
I think this file is used only for torchax-pt path.
If this is not deleted, neither can tpu_commons.models.torchax.torchax_wrapper.
Could you try which part of the file is useful for you and you check it it?
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.
@lsy323 , what do you think?
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.
Synced offline. Concluded that this file will not be removed since refactoring in #512 uses this file for torchax-jax path in favor of custom JAX attention layer. Also, other files (like tpu_commons.models.torchax.torchax_wrapper) can be deleted without any problem.
38148d4 to
86361f6
Compare
86361f6 to
ca1cab7
Compare
|
Hi @QiliangCui, as discussed, can you omit |
8a3b866 to
df6247e
Compare
Signed-off-by: Qiliang Cui <derrhein@gmail.com>
df6247e to
6a9a33f
Compare
Description
FIXES: b/438779127
Tests
Waiting for
https://buildkite.com/tpu-commons/tpu-commons-ci/builds/2396