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.

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

[air/output] Improve passed time display #34951

Merged
merged 10 commits into from
May 4, 2023

Conversation

krfricke
Copy link
Contributor

@krfricke krfricke commented May 2, 2023

Why are these changes needed?

Includes #34788

This changes the formatting of the total running time like this:

Trial easy_objective_b12ae_00003 completed training after 5 iterations at 2023-05-02 14:46:11. Total running time: 4d 3hr 20min, 2s

Trial easy_objective_d3d85_00008 completed training after 5 iterations at 2023-05-02 14:47:09. Total running time: 4d 0hr 20min 2s

Trial easy_objective_cbd93_00007 finished iteration 5 at 2023-05-02 14:46:56. Total running time: 20min 2s

Trial easy_objective_14e6b_00001 completed training after 5 iterations at 2023-05-02 14:48:58. Total running time: 1hr 0min 2s

It does not turn metrics (e.g. time_this_iter or total_time_s) into written sentences, as this will make it harder to identify these metrics for configuration purposes.

Related issue number

Closes #34922

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

@krfricke krfricke changed the title Air/output/time [air/output] Improve passed time display May 2, 2023
@krfricke krfricke requested review from gjoliver and scottsun94 May 2, 2023 14:01
@scottsun94
Copy link
Contributor

Thanks!

  1. Let's get rid of the comma and only have a space between different units? Something like 4d 3hr 20min 2s
  2. For seconds, 02.35s looks really weird.
    • When it's < 1 min, can we use 2.35s (If it's too complicated, maybe it's also okay to show 2s?)
    • When it's > 1 min, round it to integer and show 20min 2s

Kai Fricke added 5 commits May 2, 2023 20:45
Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
Signed-off-by: Kai Fricke <kai@anyscale.com>
@krfricke krfricke force-pushed the air/output/time branch from fe86407 to fe55fbb Compare May 2, 2023 19:46
@krfricke krfricke marked this pull request as ready for review May 2, 2023 19:46
Signed-off-by: Kai Fricke <kai@anyscale.com>
@krfricke
Copy link
Contributor Author

krfricke commented May 2, 2023

Thanks!

1. Let's get rid of the comma and only have a space between different units? Something like `4d 3hr 20min 2s`

2. For seconds, `02.35s` looks really weird.
   
   * When it's < 1 min, can we use `2.35s` (If it's too complicated, maybe it's also okay to show `2s`?)
   * When it's > 1 min, round it to integer and show  `20min 2s`

Makes sense! Let's go for integer seconds everywhere. I don't think there is too much value in having subsecond accuracy.

@scottsun94
Copy link
Contributor

LGTM. Thanks!

@richardliaw
Copy link
Contributor

FYI i'm not really a fan of the human-readable time, but that's ok for now. let's leave it to the users to decide.

Kai Fricke added 2 commits May 3, 2023 09:24
@krfricke krfricke added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label May 3, 2023
Copy link
Member

@gjoliver gjoliver left a comment

Choose a reason for hiding this comment

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

nice. just a couple of nits.

python/ray/tune/experimental/output.py Outdated Show resolved Hide resolved
python/ray/tune/experimental/output.py Show resolved Hide resolved
Kai Fricke added 2 commits May 4, 2023 10:13
Signed-off-by: Kai Fricke <kai@anyscale.com>
@krfricke krfricke requested a review from gjoliver May 4, 2023 09:15
@krfricke krfricke merged commit 554c1b5 into ray-project:master May 4, 2023
@krfricke krfricke deleted the air/output/time branch May 4, 2023 16:56
architkulkarni pushed a commit to architkulkarni/ray that referenced this pull request May 16, 2023
This changes the formatting of the total running time like this:

```
Trial easy_objective_b12ae_00003 completed training after 5 iterations at 2023-05-02 14:46:11. Total running time: 4d 3hr 20min, 2s

Trial easy_objective_d3d85_00008 completed training after 5 iterations at 2023-05-02 14:47:09. Total running time: 4d 0hr 20min 2s

Trial easy_objective_cbd93_00007 finished iteration 5 at 2023-05-02 14:46:56. Total running time: 20min 2s

Trial easy_objective_14e6b_00001 completed training after 5 iterations at 2023-05-02 14:48:58. Total running time: 1hr 0min 2s
```

It does not turn metrics (e.g. `time_this_iter` or `total_time_s`) into written sentences, as this will make it harder to identify these metrics for configuration purposes.

Signed-off-by: Kai Fricke <kai@anyscale.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests-ok The tagger certifies test failures are unrelated and assumes personal liability.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[AIR output] Explain what the time refers to and render time in a more readable format.
4 participants