-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Fix torch include in op_builder/mlu/fused_adam.py
and update no-torch workflow triggers
#6584
Conversation
@@ -5,7 +5,11 @@ | |||
# DeepSpeed Team | |||
|
|||
from .builder import MLUOpBuilder | |||
import torch |
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.
FYI @Andy666G - this change broke the no-torch workflow. See more info in the PR comment, but please let us know if you have any concerns with this change,
op_builder/mlu
and update no-torch workflow triggers
op_builder/mlu
and update no-torch workflow triggersop_builder/mlu/fused_adam.py
and update no-torch workflow triggers
@@ -5,7 +5,11 @@ | |||
# DeepSpeed Team | |||
|
|||
from .builder import MLUOpBuilder | |||
import torch | |||
|
|||
try: |
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 we import torch
in multi_tensor_adam
if it is the only place where you need torch?
Then you can throw a more specific error when torch is unavailable and multi_tensor_adam
is called.
Changes from #6472 caused the no-torch workflow that is an example of how we build the DeepSpeed release package to fail (so we caught this before a release, see more in #6402). These changes also copy the style used to include torch in other accelerator op_builder implementations, such as npu here and hpu here.
This also updates the no-torch workflow to run on all changes to the op_builder directory. The test runs quickly and shouldn't add any additional testing burden there.
Resolves: #6576