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

Flops profiler support einops.einsum #6755

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

lvhoaa
Copy link

@lvhoaa lvhoaa commented Nov 17, 2024

  • Added support for FlopsProfiler to include einops.einsum operation
  • Added _patch_miscellaneous_operations() and _reload_miscellaneous_operations() to include this operation and potentially include other miscellaneous operations in the future
  • Added _einops_einsum_flops_compute() that mimic already-existed _einsum_flops_compute()

@lvhoaa lvhoaa requested a review from loadams as a code owner November 17, 2024 16:56
@lvhoaa
Copy link
Author

lvhoaa commented Nov 17, 2024

@microsoft-github-policy-service agree

@@ -16,6 +16,8 @@
from deepspeed.moe.layer import MoE
from deepspeed.utils.timer import FORWARD_GLOBAL_TIMER, BACKWARD_GLOBAL_TIMER, STEP_GLOBAL_TIMER
from deepspeed.utils.torch import required_torch_version
import einops
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @lvhoaa - could you run the pre-commit formatter on the branch to ensure the formatting check passes?

Also einops is not included in the requirements file, so it will need to be added there otherwise the tests will fail.

Copy link
Author

Choose a reason for hiding this comment

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

@loadams Thanks for the feedback! I have pushed a fix!

bin/deepspeed Outdated
ds
Copy link
Contributor

Choose a reason for hiding this comment

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

@lvhoaa - I can't tell why these files were changed?

Copy link
Author

Choose a reason for hiding this comment

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

@loadams I really didn't intentionally change them. It's probably the pre-commit. Have fixed.

@@ -1,3 +1,4 @@
einops
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this would probably be better suited to the requirements-dev file

Copy link
Author

Choose a reason for hiding this comment

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

Done.

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.

2 participants