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

Add L-BFGS optimizer #2478

Merged
merged 7 commits into from
Sep 13, 2022
Merged

Add L-BFGS optimizer #2478

merged 7 commits into from
Sep 13, 2022

Conversation

jppgks
Copy link
Contributor

@jppgks jppgks commented Sep 12, 2022

No description provided.

@github-actions
Copy link

Unit Test Results

         6 files  +    1         6 suites  +1   2h 45m 57s ⏱️ + 21m 37s
  3 382 tests +    1  3 304 ✔️ +  2    78 💤 ±  0  0  - 1 
10 146 runs  +119  9 888 ✔️ +98  258 💤 +22  0  - 1 

Results for commit d66b5c9. ± Comparison against base commit 37add66.

@dantreiman dantreiman self-requested a review September 12, 2022 22:26
Copy link
Collaborator

@dantreiman dantreiman left a comment

Choose a reason for hiding this comment

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

Looks good to me. Neat!

@jppgks jppgks merged commit 5db9190 into master Sep 13, 2022
@jppgks jppgks deleted the lbfgs branch September 13, 2022 04:48
@tgaddair
Copy link
Collaborator

@jppgks we should add tests for both local and ray backends. When using the Ray backend, it should work when specifying num_workers=1, but raise an exception if num_workers>1 due to the Horovod limitation we discussed.

Based on the ray the code is written currently, I expect it will fail when calling hvd.broadcast_optimizer_state, which is what is not supported by Horovod (potentially among other things).

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.

None yet

3 participants