Skip to content
This repository has been archived by the owner on Nov 14, 2023. It is now read-only.

Allow user to pass own tune.run params in fit #212

Merged
merged 8 commits into from
Jun 17, 2021

Conversation

Yard1
Copy link
Member

@Yard1 Yard1 commented Jun 7, 2021

Will fix #211

@Yard1 Yard1 requested a review from richardliaw June 7, 2021 15:04
". This may cause unexpected issues! If you experience "
"issues, please try removing those parameters from "
"tune_params.")
warnings.warn("Using user supplied tune_params.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, if a user passes this in, they wouldn't expect a warning here right?

Copy link
Member Author

Choose a reason for hiding this comment

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

This probably should be an logging info message instead of a warning, now that I think about it

if user_overrides:
warnings.warn(
"The following preset tune.run parameters will "
f"be overriden by tune_params: {', '.join(user_overrides)}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a great error message!

@@ -626,6 +626,8 @@ def _tune_run(self, config, resources_per_trial):
resources_per_trial (dict): Resources to use per trial within Ray.
Accepted keys are `cpu`, `gpu` and custom resources, and values
are integers specifying the number of each resource to use.
tune_params (dict, optional): User defined parameters passed to
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think to get this to properly render on readthedocs, you can just leave this as (dict)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a comment here also about the overriding behavior?

@@ -626,6 +626,8 @@ def _tune_run(self, config, resources_per_trial):
resources_per_trial (dict): Resources to use per trial within Ray.
Accepted keys are `cpu`, `gpu` and custom resources, and values
are integers specifying the number of each resource to use.
tune_params (dict, optional): User defined parameters passed to
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a comment here also about the overriding behavior?

Comment on lines 757 to 758
"""Helper method to override tune.run run arguments with user
supplied dict"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit (not important here but for future ref), the first line of the docstring is a summary line - and should not have a linebreak in the middle. this is important when building docs

@richardliaw
Copy link
Collaborator

Sorry I missed this! left some simple comments. Please assign me when you get around to addressing the issues!

@Yard1 Yard1 requested a review from richardliaw June 11, 2021 10:06
@Yard1
Copy link
Member Author

Yard1 commented Jun 11, 2021

Thanks for letting me know about the docs!

@richardliaw richardliaw merged commit 3d5cf4b into ray-project:master Jun 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature request] Support resume (and potentially other tune.run args)
2 participants