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

Update mlflow.py #32972

Closed
wants to merge 5 commits into from
Closed

Update mlflow.py #32972

wants to merge 5 commits into from

Conversation

SebastianBodza
Copy link
Contributor

Removed setting the experiment_id to the ray trial id. Reordered the priority of the settings dict.

Why are these changes needed?

The MLflow and ray ID syntax is not the same. Setting the default id to the ray id just leads to additional crashes. The correct id will be set from ray - air - _internal - mlflow.py. So there is no need for a default fallback!

Adjusted the ordering of the dictionary to get first these defaults and than any fallbacks from within the function. Probably also necessary for the wandb version necessary.

Decision on how to setup credentials, servers, ... necessary. Either trough the tune dict or with additional changes in the actual trainable function itself.

Related issue number

Cleanup for #31295

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@SebastianBodza
Update mlflow.py
3eb225b 
Removed setting the experiment_id to the ray trial id

Signed-off-by: Sebastian Bodza <[email protected]>
@SebastianBodza
Update mlflow.py
381b1fe 
Reordered priority of dict

Signed-off-by: Sebastian Bodza <[email protected]>
@SebastianBodza
Update mlflow.py
06b7a31 
Added ability to change run name. 

run_name != experiment_name

Signed-off-by: Sebastian Bodza <[email protected]>
@SebastianBodza
Copy link
Contributor Author

Fixes first issues. However, when e.g. starting a MLFlow Project a mlrun is started. Then no other mlflowrun will be generated and the Script crases after the 2nd worker starts.

Calling 2x mlflow_util.log_params(_config) on the same run will lead to a crash.

@SebastianBodza
Update mlflow.py
b0ccd24 
Signed-off-by: Sebastian Bodza <[email protected]>
@SebastianBodza
Update mlflow.py
4e04b00 
Add workflow to add children in the code

Signed-off-by: Sebastian Bodza <[email protected]>
@stale
Copy link

stale bot commented Apr 2, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@stale stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Apr 2, 2023
@stale
Copy link

stale bot commented Apr 26, 2023

Hi again! The issue will be closed because there has been no more activity in the 14 days since the last message.

Please feel free to reopen or open a new issue if you'd still like it to be addressed.

Again, you can always ask for help on our discussion forum or Ray's public slack channel.

Thanks again for opening the issue!

@stale stale bot closed this Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
stale The issue is stale. It will be closed within 7 days unless there are further conversation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@SebastianBodza