[Tune] Add repr for ResultGrid class #31941

woshiyyya · 2023-01-25T23:36:39Z

Why are these changes needed?

Add repr() for ResultGrid class and prettify repr() of Result class.

The example representation:

>>> result_grid: ResultGrid = tuner.fit()
>>> result_grid
[
  Result(
    error='RayTaskError(RuntimeError)',
    metrics={'trial_id': '820f5_00000'},
    log_dir=PosixPath('/private/var/folders/0w/7cw9tmnx6nsfxtknpt99s7gc0000gn/T/pytest-of-yunxuanx/pytest-18/test_result_grid_repr0/ray_results/test_GridResults_repr/f_820f5_00000_0_x=1_2023-01-26_17-18-30'),
    checkpoint=None
  ),
  Result(
    metrics={'loss': 1, '_metadata': _CheckpointMetadata(checkpoint_type=<class 'ray.air.checkpoint.Checkpoint'>, checkpoint_state={}), 'should_checkpoint': True, 'done': True, 'trial_id': '820f5_00001', 'experiment_tag': '1_x=2'},
    log_dir=PosixPath('/private/var/folders/0w/7cw9tmnx6nsfxtknpt99s7gc0000gn/T/pytest-of-yunxuanx/pytest-18/test_result_grid_repr0/ray_results/test_GridResults_repr/f_820f5_00001_1_x=2_2023-01-26_17-18-31'),
    checkpoint=Checkpoint(local_path=/private/var/folders/0w/7cw9tmnx6nsfxtknpt99s7gc0000gn/T/pytest-of-yunxuanx/pytest-18/test_result_grid_repr0/ray_results/test_GridResults_repr/f_820f5_00001_1_x=2_2023-01-26_17-18-31/checkpoint_000000)
  ),
]

Related issue number

Closes #31716

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

justinvyu

Looks good! Just a few minor change suggestions.

justinvyu · 2023-01-26T18:00:38Z

python/ray/tune/tests/test_result_grid.py

+        with tune.checkpoint_dir(step=0) as checkpoint_dir:
+            path = os.path.join(checkpoint_dir, "checkpoint")
+            with open(path, "w") as f:
+                f.write(json.dumps({"step": 0}))
+


We can save a checkpiont with session.report. This tune.checkpoint_dir was the old API.

Suggested change

with tune.checkpoint_dir(step=0) as checkpoint_dir:

path = os.path.join(checkpoint_dir, "checkpoint")

with open(path, "w") as f:

f.write(json.dumps({"step": 0}))

python/ray/tune/tests/test_result_grid.py

Signed-off-by: Yunxuan Xiao <yunxuanx@Yunxuans-MBP.local.meter>

justinvyu

Looks good!

krfricke

I have a few minor questions for the repr implementation.

For testing, this is currently and end to end test, but I don't think this is needed. We can just create Result objects manually and a ResultGrid containing these elements. This will lead to a test execution time of a few milliseconds compared to the few seconds it takes now to run the tune experiment.

Generally we should aim to use minimalistic tests if possible - we don't get any benefit from a full end to end test here, so we should just stick with a simple unit test.

krfricke · 2023-01-28T00:21:44Z

python/ray/tune/tests/test_result_grid.py

+    tuner = tune.Tuner(
+        f,
+        run_config=air.RunConfig(
+            name="test_result_grid_repr",
+            local_dir=str(tmpdir / "test_result_grid_repr_results"),
+        ),
+        param_space={"x": tune.grid_search([1, 2])},
+    )
+    result_grid = tuner.fit()
+
+    representation = result_grid.__repr__()


Can we instead construct the result grid manually and assert the layout?

I agree. Manually creating a result grid will be way faster than running a full tuning experiment.

krfricke · 2023-01-28T00:25:29Z

python/ray/air/result.py

+        kws_repr = "  " + ",\n  ".join(kws)
+        return "{}(\n{}\n)".format(type(self).__name__, kws_repr)


It's very hard to imagine how this looks like, can we add a comment what we expect here?

krfricke · 2023-01-28T00:25:52Z

python/ray/tune/result_grid.py

+        for result in self:
+            result_repr = "  " + result.__repr__().replace("\n", "\n  ")
+            all_results_repr += f"{result_repr},\n"


Can we use a list comprehension here?

The replace looks a bit fishy. Can we maybe instead use a private method in Result for both Result.__repr__ and ResultGrid.__repr__?

krfricke · 2023-01-28T00:27:30Z

python/ray/tune/result_grid.py

+        for result in self:
+            result_repr = "  " + result.__repr__().replace("\n", "\n  ")
+            all_results_repr += f"{result_repr},\n"
+        return f"[\n{all_results_repr}]"


Should we still have ResultGrid<[ ... ]> in here?

Signed-off-by: Yunxuan Xiao <yunxuanx@Yunxuans-MBP.local.meter>

justinvyu

I have a few more small suggestions, but should be good after that! The new _results is definitely better, and +1 to creating the result objects directly in the test without running an actual experiment.

python/ray/tune/tests/test_result_grid.py

python/ray/tune/result_grid.py

python/ray/tune/tests/test_result_grid.py

Signed-off-by: Yunxuan Xiao <yunxuanx@Yunxuans-MBP.local.meter>

justinvyu

LGTM!

krfricke

Nice! There's one failing test please ping me once resolved and CI is passing

…repr

Signed-off-by: Yunxuan Xiao <yunxuanx@Yunxuans-MBP.local.meter>

Add __repr__() for ResultGrid class and prettify __repr__() of Result class. Signed-off-by: Yunxuan Xiao <yunxuanx@Yunxuans-MBP.local.meter> Co-authored-by: Yunxuan Xiao <yunxuanx@Yunxuans-MBP.local.meter> Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>

woshiyyya added the tune Tune-related issues label Jan 25, 2023

woshiyyya requested a review from justinvyu January 25, 2023 23:36

woshiyyya assigned justinvyu Jan 25, 2023

woshiyyya force-pushed the tune/result_grid_repr branch from ace2f6b to ea9a222 Compare January 26, 2023 00:12

woshiyyya marked this pull request as ready for review January 26, 2023 00:14

woshiyyya force-pushed the tune/result_grid_repr branch 2 times, most recently from 7ae7aed to b22bed7 Compare January 26, 2023 02:18

justinvyu requested changes Jan 26, 2023

View reviewed changes

[Tune] Add repr for ResultGrid class

fac8f78

Signed-off-by: Yunxuan Xiao <yunxuanx@Yunxuans-MBP.local.meter>

woshiyyya force-pushed the tune/result_grid_repr branch from b22bed7 to fac8f78 Compare January 27, 2023 01:39

justinvyu approved these changes Jan 27, 2023

View reviewed changes

woshiyyya requested a review from krfricke January 27, 2023 02:03

krfricke requested changes Jan 28, 2023

View reviewed changes

instantiate ResultGrid's result list in __init__()

6f3257b

Signed-off-by: Yunxuan Xiao <yunxuanx@Yunxuans-MBP.local.meter>

justinvyu requested changes Feb 6, 2023

View reviewed changes

fix minor issues for unit test

136b624

Signed-off-by: Yunxuan Xiao <yunxuanx@Yunxuans-MBP.local.meter>

woshiyyya requested review from justinvyu and krfricke February 6, 2023 20:03

justinvyu approved these changes Feb 6, 2023

View reviewed changes

krfricke approved these changes Feb 6, 2023

View reviewed changes

Yunxuan Xiao added 4 commits February 6, 2023 17:14

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

47800e2

…repr

fix tuner test with mock ExperimentAnalysis

ec61001

Signed-off-by: Yunxuan Xiao <yunxuanx@Yunxuans-MBP.local.meter>

fix test_result_grid_future_checkpoint

3bc5f8c

Signed-off-by: Yunxuan Xiao <yunxuanx@Yunxuans-MBP.local.meter>

fix MockExperimentAnalysis for trial_to_result

e417093

Signed-off-by: Yunxuan Xiao <yunxuanx@Yunxuans-MBP.local.meter>

woshiyyya assigned krfricke Feb 7, 2023

krfricke merged commit cf95514 into ray-project:master Feb 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Tune] Add repr for ResultGrid class #31941

[Tune] Add repr for ResultGrid class #31941

woshiyyya commented Jan 25, 2023 •

edited

Loading

justinvyu left a comment

justinvyu Jan 26, 2023

justinvyu left a comment

krfricke left a comment

krfricke Jan 28, 2023

woshiyyya Jan 28, 2023

krfricke Jan 28, 2023

krfricke Jan 28, 2023

krfricke Jan 28, 2023

justinvyu left a comment

justinvyu left a comment

krfricke left a comment

		kws_repr = " " + ",\n ".join(kws)
		return "{}(\n{}\n)".format(type(self).__name__, kws_repr)

[Tune] Add repr for ResultGrid class #31941

[Tune] Add repr for ResultGrid class #31941

Conversation

woshiyyya commented Jan 25, 2023 • edited Loading

Why are these changes needed?

Related issue number

Checks

justinvyu left a comment

Choose a reason for hiding this comment

justinvyu Jan 26, 2023

Choose a reason for hiding this comment

justinvyu left a comment

Choose a reason for hiding this comment

krfricke left a comment

Choose a reason for hiding this comment

krfricke Jan 28, 2023

Choose a reason for hiding this comment

woshiyyya Jan 28, 2023

Choose a reason for hiding this comment

krfricke Jan 28, 2023

Choose a reason for hiding this comment

krfricke Jan 28, 2023

Choose a reason for hiding this comment

krfricke Jan 28, 2023

Choose a reason for hiding this comment

justinvyu left a comment

Choose a reason for hiding this comment

justinvyu left a comment

Choose a reason for hiding this comment

krfricke left a comment

Choose a reason for hiding this comment

woshiyyya commented Jan 25, 2023 •

edited

Loading