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 integration] Don't process nan/inf results #7

Closed

Conversation

justinvyu
Copy link

@justinvyu justinvyu commented Jan 3, 2023

Signed-off-by: Justin Yu [email protected]

Currently, nan results causes the binary search happening inside SRacosTune.update_classifier to cause a stack overflow error from continuing to search recursively. This is because x > nan and x < nan is always false. This PR adds a check for invalid solution results (including nans and infs) when completing a trial.

One question: should +/- inf results be processed? Tune usually ignores these along with nans for other search algorithms.

@justinvyu justinvyu changed the title [tune integration] Donfor nan/inf results [tune integration] Don't process nan/inf results Jan 3, 2023
krfricke pushed a commit to ray-project/ray that referenced this pull request Jan 9, 2023
Skips a zoopt searcher test that's causing the `test_searchers` suite in CI to be flaky. Skipping as this is not a Tune issue and needs to be fixed in the zoopt library.

I re-enabled this test in #31147 since I thought that nan/inf error handling had been fixed in a recent zoopt release. However, nan/inf values will still cause an error if reported on trial complete.

This test should be enabled after polixir/ZOOpt#7 is included in the next zoopt release.

Signed-off-by: Justin Yu <[email protected]>
AmeerHajAli pushed a commit to ray-project/ray that referenced this pull request Jan 12, 2023
Skips a zoopt searcher test that's causing the `test_searchers` suite in CI to be flaky. Skipping as this is not a Tune issue and needs to be fixed in the zoopt library.

I re-enabled this test in #31147 since I thought that nan/inf error handling had been fixed in a recent zoopt release. However, nan/inf values will still cause an error if reported on trial complete.

This test should be enabled after polixir/ZOOpt#7 is included in the next zoopt release.

Signed-off-by: Justin Yu <[email protected]>
@justinvyu justinvyu closed this Nov 4, 2024
@justinvyu justinvyu deleted the sracos_tune_invalid_values branch November 4, 2024 20:06
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.

1 participant