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.

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

Upgrade Optuna from v2.x.x to v3.0.0 #2360

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Upgrade Optuna from v2.x.x to v3.0.0 #2360

wants to merge 4 commits into from
+38 −47

Conversation

keisuke-umezawa
Copy link

Motivation

(Write your motivation for proposed changes here.)
To upgrade Optuna from v2.x.x to v3.0.0

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

(How should this PR be tested? Do you require special setup to run the test or repro the fixed bug?)

By the existing unit tests

Related Issues and PRs

(Is this PR part of a group of changes? Link the other relevant PRs and Issues here. Use https://help.github.com/en/articles/closing-issues-using-keywords for help on GitHub syntax)
Fixes #2162

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 29, 2022
@keisuke-umezawa
Copy link
Author

Seems that the lint fails even for the main https://app.circleci.com/pipelines/github/facebookresearch/hydra/14763/workflows/e495a59a-f45c-4531-a51a-898dad566d50/jobs/146566

Jasha10
Jasha10 requested changes Sep 6, 2022
View reviewed changes
Copy link
Collaborator

@Jasha10 Jasha10 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.

Thanks @keisuke-umezawa!
Do you expect that this change to the optuna sweeper plugin will be backwards compatible for Hydra users?
Could you please bump the version in hydra_optuna_sweeper/hydra_plugins/hydra_optuna_sweeper/__init__.py to "1.3.0.dev0"?

plugins/hydra_optuna_sweeper/hydra_plugins/hydra_optuna_sweeper/_impl.py Outdated Show resolved Hide resolved
@keisuke-umezawa
Copy link
Author

@Jasha10

Thank you for your comment!

Do you expect that this change to the optuna sweeper plugin will be backwards compatible for Hydra users?

If the meaning of the backwards compatibility here is that we can run hydra with both Optuna v2.x and v3.x, we should not merge this PR. This code change can run only with Optuna v3.x.

As Optuna will keep the backwards compatibility for a while, e.g. until v4, we do not need to merge this PR at the moment. But, in that case, one note is that someone needs to merge this PR when Optuna will totally break the backwards compatibility.

@Jasha10
Copy link
Collaborator

Jasha10 commented Sep 20, 2022 •
edited
Loading

Do you expect that this change to the optuna sweeper plugin will be backwards compatible for Hydra users?

If the meaning of the backwards compatibility here is that we can run hydra with both Optuna v2.x and v3.x ...

My question is about the Optuna Sweeper's user API: will Hydra users need to update their yaml configs?
I think the answer is no; users will not need to update anything.

This code change can run only with Optuna v3.x.

This is fine :)
If users need Optuna v2.x, they can use an old version of the optuna sweeper plugin.
Edit: We are looking into usage patterns to see whether Optuna v3.x is as popular as v2.x. If not, we might delay merging this PR for a while...

@Jasha10
Copy link
Collaborator

Jasha10 commented Sep 20, 2022

I will push some commits to this PR branch to try and make the CI pass.

@Jasha10 Jasha10 mentioned this pull request Sep 20, 2022
Closed
@keisuke-umezawa
Copy link
Author

Do you expect that this change to the optuna sweeper plugin will be backwards compatible for Hydra users?

If the meaning of the backwards compatibility here is that we can run hydra with both Optuna v2.x and v3.x ...

My question is about the Optuna Sweeper's user API: will Hydra users need to update their yaml configs?
I think the answer is no; users will not need to update anything.

Your understanding is correct. They do not need to change it.

This code change can run only with Optuna v3.x.

This is fine :)
If users need Optuna v2.x, they can use an old version of the optuna sweeper plugin.
Edit: We are looking into usage patterns to see whether Optuna v3.x is as popular as v2.x. If not, we might delay merging this PR for a while...

Thank you! You can merge it when it is appropriate.

@Jasha10 Jasha10 mentioned this pull request Feb 8, 2023
Open
@AlexLup
Copy link

AlexLup commented Mar 9, 2023

It is possibile to install using pip a dev version of hydra sweeper with support for optuna 3 ?

@classner
Copy link

Could this please be merged? The 2.1 version of Optuna is very much out-of-date.

@AntonClaesson
Copy link

I'd also like this to be merged!

@himkt himkt mentioned this pull request May 25, 2023
Open
@keisuke-umezawa
Copy link
Author

@Jasha10
Long time no see and thank you for working on it!
Should I do anything to merge this PR to fix CI?

@Jasha10
Copy link
Collaborator

Jasha10 commented Jun 1, 2023

Hi @keisuke-umezawa,

Since it's been a while, it would be good to rebase this diff on top of the main branch.

Currently I'm no longer an official maintainer of Hydra. That being said, I do hope this diff can be merged at some point.

@Jasha10
Copy link
Collaborator

Jasha10 commented Jun 1, 2023 •
edited
Loading

@AlexLup

It is possibile to install using pip a dev version of hydra sweeper with support for optuna 3 ?

You can pip-install the version of hydra-optuna-sweeper from this PR

pip install 'git+https://github.com/keisuke-umezawa/hydra/@feature/fix-optuna-v3#egg=hydra-optuna-sweeper&subdirectory=plugins/hydra_optuna_sweeper'

Or you can git-clone the repo, check out @keisuke-umezawa's feature/fix-optuna-v3 branch, and run pip install --editable . in the plugins/hydra_optuna_sweeper subdirectory of the repo.

@keisuke-umezawa
Copy link
Author

@Jasha10
Sorry for being late. I rebased it against the current main branch.
Who should I talk with to merge this PR?

@Jasha10
Copy link
Collaborator

Jasha10 commented Jul 1, 2023

No problem @keisuke-umezawa. I believe that @omry and @shagunsodhani are current maintainers of Hydra.

I believe the CI failures here are unrelated to this diff. Once #2692 and #2693 are fixed, it should be possible to get the CI green.

@t-schanz
Copy link

Are there any updates on this? It seems the issues mentioned by @Jasha10 are all merged. I would love to use optuna >= 3 .

@odelalleau
Copy link
Collaborator

odelalleau commented Jul 24, 2023 •
edited
Loading

Are there any updates on this? It seems the issues mentioned by @Jasha10 are all merged. I would love to use optuna >= 3 .

In the meantime you should be able to install a version with the fix with the command in #2360 (comment)

@ItIsI-Orient ItIsI-Orient mentioned this pull request Dec 4, 2023
Open
@Jasha10 Jasha10 mentioned this pull request Jan 19, 2024
Closed
9 tasks
@shchur shchur mentioned this pull request Jan 20, 2024
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jasha10 Jasha10 Awaiting requested review from Jasha10

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrading Optuna from v2.x.x to v3.0.0
8 participants
@keisuke-umezawa @Jasha10 @AlexLup @classner @AntonClaesson @t-schanz @odelalleau @facebook-github-bot