[Tune] Don't recommend `tune.run` API in logging messages when using the `Tuner` #33642

justinvyu · 2023-03-23T18:50:01Z

Why are these changes needed?

This PR changes some logs to use the correct Tune entrypoint, depending on if the user is running with tune.run vs. tuner.fit(). Certain args like config and param_space are different between the two. Restoration logic is also different. This PR also reduces the amount of redundant logs that we print on restoration. This PR fixes the log when auto ray init happens to actually show up -- this will help users figure out how to customize ray.init options.

Problem

Before this change, doing Ctrl+C on your experiment would give you a message to restore with tune.run. Also, specifying an invalid mode in TuneConfig would also reference tune.run.

from ray import tune
import time

def train_fn(config):
    time.sleep(10)

tuner = tune.Tuner(train_fn)
tuner.fit()

... continue running with `tune.run(resume=True)`.

Auto ray init log example

Initializing Ray automatically. For cluster usage or custom Ray initialization, call `ray.init(...)` before `Tuner(...)`.

Related issue number

Closes #31478

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 added any new APIs to the API Reference. For example, if I added a
  method in Tune, I've added it in doc/source/tune/api/ under the
  corresponding .rst file.
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 :(

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>

justinvyu · 2023-03-23T18:51:19Z

python/ray/tune/tune.py

+        logger.debug(
            "TrialRunner resumed, ignoring new add_experiment but "
            "updating trial resources."
        )


What is this talking about?

Any new experiments/configurations passed to tune.run will be ignored (we only continue the current state). This is when people pass tune.run(different_experiment) when resuming.

However, overwriting trainables is now a default way

Can we maybe

Detect if an Experiment was passed or just a trainable (in this block if not isinstance(exp, Experiment))

If an experiment, continue to use the INFO message (maybe with updated wording)

Else, don't print anything

Alternatively, we can keep it as is in the PR. I don't think anybody really passes experiments anyway and the message was unhelpful to begin with.

I've kept it as a DEBUG, and improved the message a bit. It was a a bit more complicated to tell if the user passed in an experiment, since all trainables get converted to experiment -- felt that keeping it at the DEBUG log level was good enough.

woshiyyya

Looks good 🙂️

woshiyyya · 2023-03-23T19:06:26Z

python/ray/tune/tune.py

@@ -226,6 +227,7 @@ def run(
    _remote: Optional[bool] = None,
    # Passed by the Tuner.
    _remote_string_queue: Optional[Queue] = None,
+    _tuner_api: bool = False,


Is there anyway to determine the entrypoint with certain internal states, instead of passing this flag explicitly?

Hmm, it's a bit hard. I did this bc it seems like we have some special Tuner flags already, but maybe I could do a __tuner_api double underscore to make sure users really don't use this thing.

Hm I think @xwjiang2010 is also doing some context passing for entry point detection? Just checking to avoid duplicate work here

Looks like there is no telemetry for tuner vs. tune.run yet! This can be used in a future telemetry PR too then.

woshiyyya · 2023-03-23T19:16:21Z

python/ray/tune/tune.py

+        }
+        if _tuner_api
+        else {
+            "entrypoint": "tune.run(...)",


When do users typically call tune.run()?

It's the old Tune API (before Tuner) that we will deprecate at some point in the future.

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>

…/resume_vs_restore

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>

krfricke

This looks good to me, thanks! Ping me for merge

krfricke · 2023-03-24T20:36:16Z

python/ray/tune/tune.py

+        logger.debug(
            "TrialRunner resumed, ignoring new add_experiment but "
            "updating trial resources."
        )


Any new experiments/configurations passed to tune.run will be ignored (we only continue the current state). This is when people pass tune.run(different_experiment) when resuming.

However, overwriting trainables is now a default way

Can we maybe

Detect if an Experiment was passed or just a trainable (in this block if not isinstance(exp, Experiment))

If an experiment, continue to use the INFO message (maybe with updated wording)

Else, don't print anything

Alternatively, we can keep it as is in the PR. I don't think anybody really passes experiments anyway and the message was unhelpful to begin with.

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>

…the `Tuner` (ray-project#33642) Signed-off-by: Justin Yu <justinvyu@berkeley.edu> Signed-off-by: elliottower <elliot@elliottower.com>

…the `Tuner` (ray-project#33642) Signed-off-by: Justin Yu <justinvyu@berkeley.edu> Signed-off-by: Jack He <jackhe2345@gmail.com>

justinvyu added 2 commits March 23, 2023 11:09

Fix errors to distinguish tune.run vs tuner.fit

980a506

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>

Make restore logs less redundant

a28acbc

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>

justinvyu requested review from Yard1, krfricke and woshiyyya March 23, 2023 18:50

justinvyu assigned Yard1 Mar 23, 2023

justinvyu commented Mar 23, 2023

View reviewed changes

woshiyyya reviewed Mar 23, 2023

View reviewed changes

justinvyu added 3 commits March 24, 2023 09:37

fix run_experiment + make sure auto init info log shows up

9088546

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>

Merge branch 'master' of https://github.com/ray-project/ray into tune…

36e12c1

…/resume_vs_restore

small typo

93f836f

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>

krfricke approved these changes Mar 24, 2023

View reviewed changes

justinvyu added 3 commits March 24, 2023 16:08

improve message, keep debug

288653c

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>

[no_early_kickoff] merge

f850315

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>

[no_early_kickoff] merge

ec7f546

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>

justinvyu added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Mar 25, 2023

gjoliver approved these changes Mar 28, 2023

View reviewed changes

gjoliver merged commit 950fc33 into ray-project:master Mar 28, 2023

justinvyu deleted the tune/resume_vs_restore branch April 10, 2023 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Tune] Don't recommend `tune.run` API in logging messages when using the `Tuner` #33642

[Tune] Don't recommend `tune.run` API in logging messages when using the `Tuner` #33642

justinvyu commented Mar 23, 2023 •

edited

Loading

justinvyu Mar 23, 2023

krfricke Mar 24, 2023

justinvyu Mar 24, 2023

woshiyyya left a comment •

edited

Loading

woshiyyya Mar 23, 2023

justinvyu Mar 23, 2023

krfricke Mar 24, 2023

justinvyu Mar 24, 2023

woshiyyya Mar 23, 2023 •

edited

Loading

justinvyu Mar 23, 2023 •

edited

Loading

woshiyyya Mar 23, 2023

krfricke left a comment

krfricke Mar 24, 2023

[Tune] Don't recommend tune.run API in logging messages when using the Tuner #33642

[Tune] Don't recommend tune.run API in logging messages when using the Tuner #33642

Conversation

justinvyu commented Mar 23, 2023 • edited Loading

Why are these changes needed?

Problem

Auto ray init log example

Related issue number

Checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woshiyyya left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woshiyyya Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

justinvyu Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krfricke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[Tune] Don't recommend `tune.run` API in logging messages when using the `Tuner` #33642

[Tune] Don't recommend `tune.run` API in logging messages when using the `Tuner` #33642

justinvyu commented Mar 23, 2023 •

edited

Loading

woshiyyya left a comment •

edited

Loading

woshiyyya Mar 23, 2023 •

edited

Loading

justinvyu Mar 23, 2023 •

edited

Loading