[tune] Cleanup path-related properties in experiment classes #33370

krfricke · 2023-03-16T16:45:15Z

Why are these changes needed?

The naming of different path-related components in Ray Tune is currently messy. For instance, Experiment.local_dir refers to main results directory, e.g. ~/ray_results, while Trial.local_dir refers to the experiment directory, e.g. ~/ray_results/experiment. The same is true for properties, where it's unclear if it refers to the object's sub directory or to its parent directory.

To disentangle this information, this PR introduces a new naming convention.

All entities receiving a "parent path" receive it in a unambiguous naming scheme.
For instance, Experiment(storage_path), TrialRunner(experiment_path), Trial(experiment_path)
Outputs are also normalized. E.g. Trial.remote_experiment_path and Trial.local_experiment_path
We keep existing arguments and properties for backwards compatibility

One thing to discuss is the logdir property, which exists for different classes, namely Trials, Trainables, and Loggers (and some internal metadata objects).

In my opinion, logdir is unambiguous and okay. However, in the case of Trial it is the same information as local_path. The options here are a) Keep both, b) Keep logdir, c) Switch to local_path. The current version goes for c, deprecating Trial.logdir, to avoid having two names for the same thing. I'm also happy to go with option a instead. For the sake of a uniform naming convention, I would like to avoid sticking only with logdir. Any thoughts here would be appreciated.

Related issue number

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 :(

Signed-off-by: Kai Fricke <[email protected]>

xwjiang2010 · 2023-03-16T19:53:27Z

python/ray/_private/test_utils.py

@@ -1595,13 +1595,22 @@ def simulate_storage(
    elif storage_type == "s3":
        from moto.server import ThreadedMotoServer

+        old_env = os.environ


why do we need theses?

The tests don't pass locally when no aws credentials are setup. They are setup in the buildkite runners automatically (as env vars). This is just piggybacking a small fix in the current CI

python/ray/tune/experiment/config_parser.py

python/ray/tune/experiment/experiment.py

Signed-off-by: Kai Fricke <[email protected]>

…roperties

Signed-off-by: Kai Fricke <[email protected]>

xwjiang2010

unblock for the following pieces. pending test passing.

…ject#33370) The naming of different path-related components in Ray Tune is currently messy. For instance, `Experiment.local_dir` refers to main results directory, e.g. `~/ray_results`, while `Trial.local_dir` refers to the experiment directory, e.g. `~/ray_results/experiment`. The same is true for properties, where it's unclear if it refers to the object's sub directory or to its parent directory. To disentangle this information, this PR introduces a new naming convention. - All entities receiving a "parent path" receive it in a unambiguous naming scheme. - For instance, `Experiment(storage_path)`, `TrialRunner(experiment_path)`, `Trial(experiment_path)` - Outputs are also normalized. E.g. `Trial.remote_experiment_path` and `Trial.local_experiment_path` - We keep existing arguments and properties for backwards compatibility Signed-off-by: Kai Fricke <[email protected]> Signed-off-by: Jack He <[email protected]>

…ject#33370) The naming of different path-related components in Ray Tune is currently messy. For instance, `Experiment.local_dir` refers to main results directory, e.g. `~/ray_results`, while `Trial.local_dir` refers to the experiment directory, e.g. `~/ray_results/experiment`. The same is true for properties, where it's unclear if it refers to the object's sub directory or to its parent directory. To disentangle this information, this PR introduces a new naming convention. - All entities receiving a "parent path" receive it in a unambiguous naming scheme. - For instance, `Experiment(storage_path)`, `TrialRunner(experiment_path)`, `Trial(experiment_path)` - Outputs are also normalized. E.g. `Trial.remote_experiment_path` and `Trial.local_experiment_path` - We keep existing arguments and properties for backwards compatibility Signed-off-by: Kai Fricke <[email protected]> Signed-off-by: Edward Oakes <[email protected]>

…ject#33370) The naming of different path-related components in Ray Tune is currently messy. For instance, `Experiment.local_dir` refers to main results directory, e.g. `~/ray_results`, while `Trial.local_dir` refers to the experiment directory, e.g. `~/ray_results/experiment`. The same is true for properties, where it's unclear if it refers to the object's sub directory or to its parent directory. To disentangle this information, this PR introduces a new naming convention. - All entities receiving a "parent path" receive it in a unambiguous naming scheme. - For instance, `Experiment(storage_path)`, `TrialRunner(experiment_path)`, `Trial(experiment_path)` - Outputs are also normalized. E.g. `Trial.remote_experiment_path` and `Trial.local_experiment_path` - We keep existing arguments and properties for backwards compatibility Signed-off-by: Kai Fricke <[email protected]>

Following #33370, this PR adds `Result.path` and `ResultGrid.experiment_path` to the respective classes. Further, we remove the public facing `ExperimentAnalysis.local_path` and `ExperimentAnalysis.remote_path` in favor of a unified `ExperimentAnalysis.experiment_path`. Signed-off-by: Kai Fricke <[email protected]>

…ject#33370) The naming of different path-related components in Ray Tune is currently messy. For instance, `Experiment.local_dir` refers to main results directory, e.g. `~/ray_results`, while `Trial.local_dir` refers to the experiment directory, e.g. `~/ray_results/experiment`. The same is true for properties, where it's unclear if it refers to the object's sub directory or to its parent directory. To disentangle this information, this PR introduces a new naming convention. - All entities receiving a "parent path" receive it in a unambiguous naming scheme. - For instance, `Experiment(storage_path)`, `TrialRunner(experiment_path)`, `Trial(experiment_path)` - Outputs are also normalized. E.g. `Trial.remote_experiment_path` and `Trial.local_experiment_path` - We keep existing arguments and properties for backwards compatibility Signed-off-by: Kai Fricke <[email protected]> Signed-off-by: chaowang <[email protected]>

…ject#33370) The naming of different path-related components in Ray Tune is currently messy. For instance, `Experiment.local_dir` refers to main results directory, e.g. `~/ray_results`, while `Trial.local_dir` refers to the experiment directory, e.g. `~/ray_results/experiment`. The same is true for properties, where it's unclear if it refers to the object's sub directory or to its parent directory. To disentangle this information, this PR introduces a new naming convention. - All entities receiving a "parent path" receive it in a unambiguous naming scheme. - For instance, `Experiment(storage_path)`, `TrialRunner(experiment_path)`, `Trial(experiment_path)` - Outputs are also normalized. E.g. `Trial.remote_experiment_path` and `Trial.local_experiment_path` - We keep existing arguments and properties for backwards compatibility Signed-off-by: Kai Fricke <[email protected]> Signed-off-by: elliottower <[email protected]>

…t#33410) Following ray-project#33370, this PR adds `Result.path` and `ResultGrid.experiment_path` to the respective classes. Further, we remove the public facing `ExperimentAnalysis.local_path` and `ExperimentAnalysis.remote_path` in favor of a unified `ExperimentAnalysis.experiment_path`. Signed-off-by: Kai Fricke <[email protected]> Signed-off-by: elliottower <[email protected]>

…ject#33370) The naming of different path-related components in Ray Tune is currently messy. For instance, `Experiment.local_dir` refers to main results directory, e.g. `~/ray_results`, while `Trial.local_dir` refers to the experiment directory, e.g. `~/ray_results/experiment`. The same is true for properties, where it's unclear if it refers to the object's sub directory or to its parent directory. To disentangle this information, this PR introduces a new naming convention. - All entities receiving a "parent path" receive it in a unambiguous naming scheme. - For instance, `Experiment(storage_path)`, `TrialRunner(experiment_path)`, `Trial(experiment_path)` - Outputs are also normalized. E.g. `Trial.remote_experiment_path` and `Trial.local_experiment_path` - We keep existing arguments and properties for backwards compatibility Signed-off-by: Kai Fricke <[email protected]> Signed-off-by: Jack He <[email protected]>

…t#33410) Following ray-project#33370, this PR adds `Result.path` and `ResultGrid.experiment_path` to the respective classes. Further, we remove the public facing `ExperimentAnalysis.local_path` and `ExperimentAnalysis.remote_path` in favor of a unified `ExperimentAnalysis.experiment_path`. Signed-off-by: Kai Fricke <[email protected]> Signed-off-by: Jack He <[email protected]>

Kai Fricke added 5 commits March 16, 2023 09:44

[tune] Cleanup path-related properties in experiment classes

2a34908

Signed-off-by: Kai Fricke <[email protected]>

Leftovers from trial runner / experiment analysis

a1cc7cb

Signed-off-by: Kai Fricke <[email protected]>

Integration tests

00704f7

Signed-off-by: Kai Fricke <[email protected]>

some test fixes

ed317d4

Signed-off-by: Kai Fricke <[email protected]>

Mock trial fixes in test_logger.py

6eee2cb

Signed-off-by: Kai Fricke <[email protected]>

krfricke marked this pull request as ready for review March 16, 2023 18:52

krfricke assigned justinvyu and xwjiang2010 Mar 16, 2023

xwjiang2010 reviewed Mar 16, 2023

View reviewed changes

Kai Fricke added 4 commits March 16, 2023 14:24

fix bug in experiment dir name overwriting

1b684d7

Signed-off-by: Kai Fricke <[email protected]>

Makedirs

6c69033

Signed-off-by: Kai Fricke <[email protected]>

Comments

e74da9c

Signed-off-by: Kai Fricke <[email protected]>

Merge remote-tracking branch 'upstream/master' into tune/experiment-p…

00a9d37

…roperties

krfricke requested a review from xwjiang2010 March 16, 2023 23:03

Kai Fricke added 2 commits March 16, 2023 16:29

revert

fca99e9

Signed-off-by: Kai Fricke <[email protected]>

revert

e34669c

Signed-off-by: Kai Fricke <[email protected]>

xwjiang2010 approved these changes Mar 17, 2023

View reviewed changes

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

krfricke merged commit 862a5fb into ray-project:master Mar 17, 2023

krfricke deleted the tune/experiment-properties branch March 17, 2023 02:32

krfricke mentioned this pull request Mar 17, 2023

[air] Add *path properties to Result and ResultGrid #33410

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tune] Cleanup path-related properties in experiment classes #33370

[tune] Cleanup path-related properties in experiment classes #33370

krfricke commented Mar 16, 2023 •

edited

Loading

xwjiang2010 Mar 16, 2023

krfricke Mar 16, 2023

xwjiang2010 left a comment

[tune] Cleanup path-related properties in experiment classes #33370

[tune] Cleanup path-related properties in experiment classes #33370

Conversation

krfricke commented Mar 16, 2023 • edited Loading

Why are these changes needed?

Related issue number

Checks

xwjiang2010 Mar 16, 2023

Choose a reason for hiding this comment

krfricke Mar 16, 2023

Choose a reason for hiding this comment

xwjiang2010 left a comment

Choose a reason for hiding this comment

krfricke commented Mar 16, 2023 •

edited

Loading