-
Notifications
You must be signed in to change notification settings - Fork 7.1k
setup_mlflow API Ray Train V2 Compatibility #58705
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
setup_mlflow API Ray Train V2 Compatibility #58705
Conversation
…p_mlflow Signed-off-by: JasonLi1909 <jasli1909@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request aims to add compatibility with Ray Train V2 for the setup_mlflow API by handling deprecation warnings. The changes introduce try-except blocks to catch DeprecationWarning and fall back to alternative APIs or gracefully handle the situation.
While the intent is good, I've found a critical issue where the new structure could lead to a NameError if ray.train.get_context() raises a RuntimeError. I've provided a suggestion to restructure the code to prevent this.
Additionally, I've pointed out that simply catching DeprecationWarning for context methods like get_world_rank() and then proceeding without trial information could lead to silent failures in MLflow logging. It would be better to use the newer APIs if possible.
Signed-off-by: JasonLi1909 <jasli1909@gmail.com>
Signed-off-by: JasonLi1909 <jasli1909@gmail.com>
Signed-off-by: JasonLi1909 <jasli1909@gmail.com>
Signed-off-by: JasonLi1909 <jasli1909@gmail.com>
Signed-off-by: JasonLi1909 <jasli1909@gmail.com>
Signed-off-by: JasonLi1909 <jasli1909@gmail.com>
justinvyu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, just a few minor things
Signed-off-by: JasonLi1909 <jasli1909@gmail.com>
Signed-off-by: JasonLi1909 <jasli1909@gmail.com>
Signed-off-by: JasonLi1909 <jasli1909@gmail.com>
Signed-off-by: JasonLi1909 <jasli1909@gmail.com>
justinvyu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Co-authored-by: Justin Yu <justinvyu@anyscale.com> Signed-off-by: Jason Li <57246540+JasonLi1909@users.noreply.github.com>
- Makes setup_mlflow compatible with Ray Train V2 by avoiding deprecated API usage - Adds tests for testing the compatibility of Train V2 with setup_mlflow --------- Signed-off-by: JasonLi1909 <jasli1909@gmail.com> Signed-off-by: Jason Li <57246540+JasonLi1909@users.noreply.github.com> Co-authored-by: Justin Yu <justinvyu@anyscale.com>
- Makes setup_mlflow compatible with Ray Train V2 by avoiding deprecated API usage - Adds tests for testing the compatibility of Train V2 with setup_mlflow --------- Signed-off-by: JasonLi1909 <jasli1909@gmail.com> Signed-off-by: Jason Li <57246540+JasonLi1909@users.noreply.github.com> Co-authored-by: Justin Yu <justinvyu@anyscale.com> Signed-off-by: YK <1811651+ykdojo@users.noreply.github.com>
- Makes setup_mlflow compatible with Ray Train V2 by avoiding deprecated API usage - Adds tests for testing the compatibility of Train V2 with setup_mlflow --------- Signed-off-by: JasonLi1909 <jasli1909@gmail.com> Signed-off-by: Jason Li <57246540+JasonLi1909@users.noreply.github.com> Co-authored-by: Justin Yu <justinvyu@anyscale.com>
Users have run into issues using the
setup_mlflowAPI with Ray Train V2 enabled:setup_mlflowin a tune run, aDeprecationWarningis raised becauseray.train.get_context()is called from with a Ray Tune runsetup_mlflowin a regular Train run, aDeprecationWarningis raised becauseTrainContext.get_trial_id()is called but this is no longer supported in Train V2This PR: