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] OptunaSearch: check compatibility of search space with evaluated_rewards #18625

Merged
merged 8 commits into from
Sep 23, 2021
Merged

[tune] OptunaSearch: check compatibility of search space with evaluated_rewards #18625

merged 8 commits into from
Sep 23, 2021

Conversation

ccssmnn
Copy link
Contributor

@ccssmnn ccssmnn commented Sep 15, 2021

Why are these changes needed?

OptunaSearch accepts both distribution dictionary or define-by-run as a definition for the search space. The latter should not be used, when evaluated_rewards is provided. Creation of trials in Optuna only allows the distribution strategy.

This PR adds docs for this case and provides a meaningful error when define-by-run is used with evaluated_rewards

Related issue number

Closes #18587

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

I need some guidance how to run the tests. Is there guidance on how to execute the tests? Installing the dependencies inside the python folder and running python -m unittest fails to execute the tests: ImportError: Failed to import test module: ray.

@ccssmnn
Copy link
Contributor Author

ccssmnn commented Sep 15, 2021

@Yard1

@ccssmnn ccssmnn marked this pull request as draft September 15, 2021 08:52
@Yard1 Yard1 self-assigned this Sep 15, 2021
@Yard1 Yard1 self-requested a review September 15, 2021 10:21
@Yard1
Copy link
Member

Yard1 commented Sep 15, 2021

@ccssmnn Lint is still failing with python/ray/tune/tests/test_searchers.py:310:80: E501 line too long (83 > 79 characters), can you check? Also, please merge the latest upstream master into your branch, there have been some CI issues that were fixed.

As for running tests locally, you should create a new virutalenv, follow instructions on how to setup ray python dev environment and then python/requirements/requirements_default.txt as well as python/requirements/tune/requirements_tune.txt. That should allow you to run tests without issues with pytest TEST_FILE_NAME.py

Copy link
Member

@Yard1 Yard1 left a comment

Choose a reason for hiding this comment

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

There is a lot of unrelated code being reformatted. Can you ensure that you are using the right yapf version for scripts/format.sh, and that only related code is commited? Thanks!

@@ -54,7 +54,6 @@ class _OptunaTrialSuggestCaptor:
`suggest_` callables with a function capturing the returned value,
which will be saved in the ``captured_values`` dict.
"""

def __init__(self, ot_trial: OptunaTrial) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Can we leave an empty line here?

@@ -126,6 +125,12 @@ class OptunaSearch(Searcher):
needing to re-compute the trial. Must be the same length as
points_to_evaluate.

..warning::
When using evaluated_rewards, the search space `space` must
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
When using evaluated_rewards, the search space `space` must
When using ``evaluated_rewards``, the search space ``space`` must

Comment on lines 131 to 132
``optuna.distributions``. The define-by-run definition is not
yet supported.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
``optuna.distributions``. The define-by-run definition is not
yet supported.
``optuna.distributions`` instances as values. The define-by-run
search space definition is not yet supported with this functionality.

python/ray/tune/suggest/optuna.py Show resolved Hide resolved
Copy link
Member

@Yard1 Yard1 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! Just one last nit. Feel free to mark it as ready for review.

Co-authored-by: Antoni Baum <[email protected]>
@ccssmnn
Copy link
Contributor Author

ccssmnn commented Sep 16, 2021

Thanks @Yard1 !

I'm still running into errors executing the test locally though.

My steps:

  1. create a GitHub CodeSpace from this PR
  2. remove .venv which is created by vscode
  3. create a new venv with python -m venv venv
  4. activate venv source venv/bin/activate
  5. install the latest ray wheels with pip command as suggested in the URL you provided
  6. run python python/ray/setup-dev.py
  7. install dependencies with pip install -r python/requirements.txt
  8. run test pytest python/ray/tune/tests/test_searchers.py

Results in the following error:

(venv) codespace ➜ /workspaces/ray (pr/ccssmnn/18625 ✗) $ pytest python/ray/tune/tests/test_searchers.py 
Test session starts (platform: linux, Python 3.8.6, pytest 5.4.3, pytest-sugar 0.9.4)
rootdir: /workspaces/ray/python
plugins: lazy-fixture-0.6.3, sugar-0.9.4, rerunfailures-10.1, anyio-3.3.1, asyncio-0.15.1, timeout-1.4.2
collecting ... 
―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ERROR collecting ray/tune/tests/test_searchers.py ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
python/ray/tune/tests/test_searchers.py:5: in <module>
    import ray
venv/lib/python3.8/site-packages/ray/__init__.py:89: in <module>
    from ray.worker import (  # noqa: E402,F401
venv/lib/python3.8/site-packages/ray/worker.py:24: in <module>
    import ray._private.parameter
venv/lib/python3.8/site-packages/ray/_private/parameter.py:11: in <module>
    class RayParams:
venv/lib/python3.8/site-packages/ray/_private/parameter.py:153: in RayParams
    worker_setup_hook=ray_constants.DEFAULT_WORKER_SETUP_HOOK,
E   AttributeError: module 'ray.ray_constants' has no attribute 'DEFAULT_WORKER_SETUP_HOOK'

=================================================================================== short test summary info ===================================================================================
FAILED python/ray/tune/tests/test_searchers.py - AttributeError: module 'ray.ray_constants' has no attribute 'DEFAULT_WORKER_SETUP_HOOK'
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

Results (0.42s):

This may be irrelevant for this PR, but it can be a blocker for future PRs.

@ccssmnn ccssmnn marked this pull request as ready for review September 19, 2021 11:02
@Yard1
Copy link
Member

Yard1 commented Sep 21, 2021

@ccssmnn Hey, sorry to keep you waiting. The workflow you have posted is pretty much exactly what I am doing. Not sure why it would cause such an error for you. Can you try again?

I will merge master into your branch to make CI pass.

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.

Thanks!

@krfricke krfricke merged commit 882f7d3 into ray-project:master Sep 23, 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]: OptunaSearch define-by-run space incompatible with evaluated_rewards
3 participants