Skip to content

Generalize Optimizers container type, by passing base internal optimizer class. #884

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.

Sign up for GitHub

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

Jump to bottom
Merged
tianyu-l merged 1 commit into from
Feb 25, 2025
Merged

Generalize Optimizers container type, by passing base internal optimizer class. #884

tianyu-l merged 1 commit into from
Feb 25, 2025

Conversation

@balancap
Copy link
Contributor

@balancap balancap commented Feb 24, 2025

Passing optimizer_cls to OptimizersContainer and OptimizersInBackwardContainer constructors, instead of name.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Feb 24, 2025
@balancap
Copy link
Contributor Author

balancap commented Feb 24, 2025 •
edited
Loading

As discussed in #876 , generalizing OptimizersContainer to enable re-use by users with different type of optimizers.

Additionally, I removed _create_optimizer, directly integrating the small Optimizer dict in the build function.

tianyu-l
tianyu-l reviewed Feb 25, 2025
View reviewed changes
Copy link
Contributor

@tianyu-l tianyu-l left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM!

  1. Please fix CI breakage.
  2. Could you rebase, since #881 has been merged?

@balancap
Generalize Optimizers container type, by passing base internal optimi…
99cc449 
…zer class.

Passing `optimizer_cls` to `OptimizersContainer` and `OptimizersInBackwardContainer` constructors, instead of `name`.
@balancap balancap force-pushed the generalize-optimizers-container branch from 2523768 to 99cc449 Compare February 25, 2025 15:03
@balancap
Copy link
Contributor Author

balancap commented Feb 25, 2025

Thanks @tianyu-l , should be good now after rebase. My bad forgotting to run the unit tests before pushing the final commit!

@tianyu-l tianyu-l merged commit 82629f8 into pytorch:main Feb 25, 2025
6 checks passed
MaxiBoether pushed a commit to eth-easl/torchtitan-mixtera that referenced this pull request Apr 17, 2025
@balancap @MaxiBoether
Generalize Optimizers container type, by passing base internal optimi…
ce21f1b 
…zer class. (pytorch#884)

Passing `optimizer_cls` to `OptimizersContainer` and
`OptimizersInBackwardContainer` constructors, instead of `name`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tianyu-l tianyu-l tianyu-l left review comments

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@balancap @tianyu-l @facebook-github-bot