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

Fix torch include in op_builder/mlu/fused_adam.py and update no-torch workflow triggers #6584

Merged
merged 5 commits into from
Sep 27, 2024

Conversation

loadams
Copy link
Contributor

@loadams loadams commented Sep 27, 2024

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

@loadams loadams changed the title Update triggers for no-torch test Fix torch include in op_builder/mlu and update no-torch workflow triggers Sep 27, 2024
@loadams loadams requested a review from tohtana September 27, 2024 16:59
@@ -5,7 +5,11 @@
# DeepSpeed Team

from .builder import MLUOpBuilder
import torch
Copy link
Contributor Author

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,

@loadams loadams enabled auto-merge September 27, 2024 17:09
@loadams loadams requested a review from jomayeri September 27, 2024 17:10
@loadams loadams changed the title Fix torch include in op_builder/mlu and update no-torch workflow triggers Fix torch include in op_builder/mlu and update no-torch workflow triggers Sep 27, 2024
@loadams loadams changed the title Fix torch include in op_builder/mlu and update no-torch workflow triggers Fix torch include in op_builder/mlu/fused_adam.py and update no-torch workflow triggers Sep 27, 2024
@@ -5,7 +5,11 @@
# DeepSpeed Team

from .builder import MLUOpBuilder
import torch

try:
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 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.

@loadams loadams disabled auto-merge September 27, 2024 20:32
@loadams loadams merged commit 8cded57 into master Sep 27, 2024
10 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

no-torch CI test failure
3 participants