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

Align run name strategy between MLflowCallback and track_in_mlflow method #111

Merged
merged 5 commits into from
Apr 22, 2024
Merged

Align run name strategy between MLflowCallback and track_in_mlflow method #111

merged 5 commits into from
Apr 22, 2024

Conversation

TTRh
Copy link
Contributor

@TTRh TTRh commented Apr 9, 2024 •
edited
Loading

Motivation

The strategy for infering the run name was not aligned between the base class MLflowCallback and the decorator track_in_mlflow.

Description of the changes

This PR aligned the strategy to get the run name from MLfowCallback and pass it to mlflow. We lookup _mlflow_kwargs and we fallback on trial.number if not given.

@TTRh TTRh changed the title Align run name strategy between MLflowCallback and track_in_mlflow me… Align run name strategy between MLflowCallback and track_in_mlflow method Apr 9, 2024
@TTRh TTRh mentioned this pull request Apr 9, 2024
Closed
@TTRh
Copy link
Contributor Author

TTRh commented Apr 9, 2024

Tests are failing but i'm not sure it's related to my change i see in the logs:

ERROR collecting tests/lightgbm_tuner_tests/test_alias.py

@HideakiImamura
Copy link
Member

Sorry for the confusion. I will fix the main branch. Please wait a minuite.

@HideakiImamura HideakiImamura self-assigned this Apr 11, 2024
@HideakiImamura
Copy link
Member

The main branch has been fixed in #112. Could you merge the main branch?

nzw0301
nzw0301 reviewed Apr 11, 2024
View reviewed changes
optuna_integration/mlflow.py Outdated
@@ -207,8 +207,9 @@ def wrapper(trial: optuna.trial.Trial) -> float | Sequence[float]:
study = trial.study
self._initialize_experiment(study)
nested = self._mlflow_kwargs.get("nested")
run_name = self._mlflow_kwargs.get("run_name") or str(trial.number)
Copy link
Member

Choose a reason for hiding this comment

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

If the first value can be evaluated as False although I think this happens very unlikely, str(trial.number) is used, so how about using str(trial.number) as a default?

Suggested change
run_name = self._mlflow_kwargs.get("run_name") or str(trial.number)
run_name = self._mlflow_kwargs.get("run_name", str(trial.number))

Copy link
Contributor Author

@TTRh TTRh Apr 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

i wanted to have the exact same behavior as in the MLFlowCallback call method but ok to change 👍

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right. Thank you for your comment. Maybe we can update MLFlowCallback's part too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done :)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. Sorry, the mlflow test failed due to my suggestion, let me check what was wrong...

Copy link
Contributor Author

@TTRh TTRh Apr 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

yep i updated the test because we were setting run_name at None in the test. If it's ok i adjusted it like that: 1296daa

Copy link
Member

Choose a reason for hiding this comment

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

Awesome!

nzw0301
nzw0301 approved these changes Apr 11, 2024
View reviewed changes
Copy link
Member

@nzw0301 nzw0301 left a comment

Choose a reason for hiding this comment

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

Thanks! Personally, the change looks great to me.

@github-actions GitHub Actions
Copy link

This pull request has not seen any recent activity.

@github-actions github-actions bot added the stale Exempt from stale bot labeling. label Apr 18, 2024
@eukaryo eukaryo self-assigned this Apr 22, 2024
@eukaryo eukaryo added code-fix Change that does not change the behavior, such as code refactoring. and removed stale Exempt from stale bot labeling. labels Apr 22, 2024
eukaryo
eukaryo approved these changes Apr 22, 2024
View reviewed changes
Copy link
Contributor

@eukaryo eukaryo left a comment

Choose a reason for hiding this comment

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

I have reviewed this PR. LGTM. Since the assigned reviewer is currently busy, I will proceed with merging.

@eukaryo eukaryo merged commit 1c590a8 into optuna:main Apr 22, 2024
12 checks passed
@eukaryo eukaryo added this to the v4.0.0 milestone Apr 22, 2024
@not522 not522 changed the title Align run name strategy between MLflowCallback and track_in_mlflow method Align run name strategy between MLflowCallback and track_in_mlflow method Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eukaryo eukaryo eukaryo approved these changes

@nzw0301 nzw0301 nzw0301 approved these changes

Assignees
No one assigned
Labels
code-fix Change that does not change the behavior, such as code refactoring.
Projects
None yet
Milestone
v4.0.0-b0
Development

Successfully merging this pull request may close these issues.
4 participants
@TTRh @HideakiImamura @nzw0301 @eukaryo