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

[tune] Sort top results by metric #16576

Merged
merged 11 commits into from
Jul 6, 2021

Conversation

Eleven1Liu
Copy link
Contributor

@Eleven1Liu Eleven1Liu commented Jun 21, 2021

Why are these changes needed?

Make the reporter show the top results sorted by metric.

Related issue number

Closes #16448 (@krfricke)

Checks

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

@krfricke krfricke self-assigned this Jun 21, 2021
@krfricke
Copy link
Contributor

Thanks for this effort! A couple of comments from a first glance:

  1. Could you add a feature flag to the TuneReporterBase base class to enable/disable sorting? Since it changes the default behavior, users should explicitly enable sorting by passing something like sort_by_metric=True to CLIReporter/JupyterNotebookReporter.

  2. It would be great if we could leave the tests as they are - but please feel free to add a new one or a variation of an existing one. E.g. use the same test, one time with sort_by_metric=True and one time without.

@Eleven1Liu Eleven1Liu changed the title [WIP][tune] Sort top results by metric [tune] Sort top results by metric Jun 28, 2021
@Eleven1Liu
Copy link
Contributor Author

Eleven1Liu commented Jun 28, 2021

@krfricke Thanks for the suggestions! I have:

  1. added a feature flag sort_by_metric to TuneReporterBase.
  2. added new test testSortByMetric in test_progress_reporter.py.

It seems some of the flaky tests do not passed.

@krfricke
Copy link
Contributor

@Eleven1Liu, this is fantastic! Thanks! The only thing I'd change is in the test. Currently, the EXPECTED_SORT_RESULT_1 seems to test the unsorted results table. However, it is in fact sorted in an ascending way (because scores increase with trial ID).

I think the following would make sense:

Trial 0 --> 0.3
Trial 1 --> 0.2
Trial 2 --> 0.4

So then we get these orders (that we can test for):

Unsorted: 0 - 1 - 2
Sorted (Asc): 1 - 0 - 2
Sorted (Desc): 2 - 0 - 1

What do you think?

@Eleven1Liu
Copy link
Contributor Author

Sounds great! Let me check the code.

@Eleven1Liu
Copy link
Contributor Author

Hi @krfricke,
I've updated the test cases (unsorted, asc, and desc).

Copy link
Contributor

@krfricke krfricke left a comment

Choose a reason for hiding this comment

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

This is amazing. Thank you very much for this contribution and the revisions!

@krfricke krfricke merged commit e250abf into ray-project:master Jul 6, 2021
@Eleven1Liu
Copy link
Contributor Author

Hi @krfricke,

Wow, great! Thanks for the suggestions and guidance.
I'm happy to have contributed and write code with you!! 😁

jiaodong pushed a commit that referenced this pull request Jul 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[tune] Sort top results by metric
2 participants