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] Add OSS Vizier to Ray Tune #48684

Open
wants to merge 59 commits into
base: master
Choose a base branch
from

Conversation

xingyousong
Copy link

@xingyousong xingyousong commented Nov 11, 2024

Why are these changes needed?

We're adding OSS Vizier (https://github.com/google/vizier) to Ray Tune to increase reach and allow Ray users to use Google's hyperparameter optimizer, since it's been experimentally benchmarked to be one of the best.

Related issue number

None

Checks

  • YES: I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • NO (I edited everything in Github GUI) I've run scripts/format.sh to lint the changes in this PR.
  • N/A I've included any doc changes needed for https://docs.ray.io/en/master/.
    • YES 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.
  • NO (I added tests but don't know how to install RayTune with a custom fork) 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
    • YES: Unit tests
    • YES: Release tests
    • NO: This PR is not tested :(

Signed-off-by: xingyousong <[email protected]>
Signed-off-by: xingyousong <[email protected]>
Signed-off-by: xingyousong <[email protected]>
Signed-off-by: xingyousong <[email protected]>
Signed-off-by: xingyousong <[email protected]>
Signed-off-by: xingyousong <[email protected]>
Signed-off-by: xingyousong <[email protected]>
Signed-off-by: xingyousong <[email protected]>
Signed-off-by: xingyousong <[email protected]>
Signed-off-by: xingyousong <[email protected]>
Signed-off-by: xingyousong <[email protected]>
Signed-off-by: xingyousong <[email protected]>
Signed-off-by: xingyousong <[email protected]>
Signed-off-by: xingyousong <[email protected]>
Signed-off-by: xingyousong <[email protected]>
Signed-off-by: xingyousong <[email protected]>
Signed-off-by: xingyousong <[email protected]>
Signed-off-by: xingyousong <[email protected]>
Signed-off-by: xingyousong <[email protected]>
Signed-off-by: xingyousong <[email protected]>
@xingyousong xingyousong changed the title Add OSS Vizier to Ray Tune [Tune] Add OSS Vizier to Ray Tune Nov 11, 2024
@jcotant1 jcotant1 added the tune Tune-related issues label Nov 15, 2024
@matthewdeng matthewdeng self-assigned this Nov 15, 2024
Copy link
Contributor

@matthewdeng matthewdeng left a comment

Choose a reason for hiding this comment

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

Awesome work!

Added an initial round of comments.

python/ray/tune/search/vizier/vizier_search.py Outdated Show resolved Hide resolved
python/ray/tune/search/vizier/vizier_search.py Outdated Show resolved Hide resolved
python/ray/tune/search/vizier/vizier_search.py Outdated Show resolved Hide resolved
python/ray/tune/search/vizier/vizier_search.py Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any more tests you'd recommend running/adding to ensure that all the Vizier functionality is properly supported?

Copy link
Author

Choose a reason for hiding this comment

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

Good question - but first asking, is there any easy way where I can actually test these on my local machine?

I think that's equivalent to: How to install Ray[tune] from a PR, rather than pip?

ATM I've actually never ran any of these tests myself, just copy pasted + eyeballed from other algorithms' tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can follow the instructions here (main thing is installing with pip install -e . from the /ray/python directory).

Copy link
Author

Choose a reason for hiding this comment

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

I'm having a lot of trouble installing manually - my most recent error was:

exists. Specify --upgrade to force replacement.
    WARNING: Target directory /usr/local/google/home/xingyousong/ray/python/ray/_private/runtime_env/agent/thirdparty_files/aiosignal-1.3.2.dist-info already exists. Specify --upgrade to force replacement.
    WARNING: Target directory /usr/local/google/home/xingyousong/ray/python/ray/_private/runtime_env/agent/thirdparty_files/aiohttp-3.11.11.dist-info already exists. Specify --upgrade to force replacement.
    WARNING: Target directory /usr/local/google/home/xingyousong/ray/python/ray/_private/runtime_env/agent/thirdparty_files/aiohttp already exists. Specify --upgrade to force replacement.
    error: [Errno 2] No such file or directory: '/usr/local/google/home/xingyousong/bin/bazel'
    error: subprocess-exited-with-error
    
    × python setup.py develop did not run successfully.
    │ exit code: 1
    ╰─> See above for output.
    
    note: This error originates from a subprocess, and is likely not a problem with pip.
    full command: /usr/bin/python3 -c '
    exec(compile('"'"''"'"''"'"'
    # This is <pip-setuptools-caller> -- a caller that pip uses to run setup.py
    #
    # - It imports setuptools before invoking setup.py, to enable projects that directly
    #   import from `distutils.core` to work with newer packaging standards.
    # - It provides a clear error message when setuptools is not installed.
    # - It sets `sys.argv[0]` to the underlying `setup.py`, when invoking `setup.py` so
    #   setuptools doesn'"'"'t think the script is `-c`. This avoids the following warning:
    #     manifest_maker: standard file '"'"'-c'"'"' not found".
    # - It generates a shim setup.py, for handling setup.cfg-only projects.
    import os, sys, tokenize
    
    try:
        import setuptools
    except ImportError as error:
        print(
            "ERROR: Can not execute `setup.py` since setuptools is not available in "
            "the build environment.",
            file=sys.stderr,
        )
        sys.exit(1)
    
    __file__ = %r
    sys.argv[0] = __file__
    
    if os.path.exists(__file__):
        filename = __file__
        with tokenize.open(__file__) as f:
            setup_py_code = f.read()
    else:
        filename = "<auto-generated setuptools caller>"
        setup_py_code = "from setuptools import setup; setup()"
    
    exec(compile(setup_py_code, filename, "exec"))
    '"'"''"'"''"'"' % ('"'"'/usr/local/google/home/xingyousong/ray/python/setup.py'"'"',), "<pip-setuptools-caller>", "exec"))' develop --no-deps --user --prefix=
    cwd: /usr/local/google/home/xingyousong/ray/python/
error: subprocess-exited-with-error

× python setup.py develop did not run successfully.
│ exit code: 1
╰─> See above for output.

when I tried pip install -e . actually. For now I'm just gonna rely on these Github workflows (e.g. Microcheck) to guide the debugging...

Copy link
Contributor

Choose a reason for hiding this comment

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

You can follow the instructions here (main thing is installing with pip install -e . from the /ray/python directory).

python/ray/tune/search/vizier/vizier_search.py Outdated Show resolved Hide resolved
python/ray/tune/search/vizier/vizier_search.py Outdated Show resolved Hide resolved
python/ray/tune/search/vizier/vizier_search.py Outdated Show resolved Hide resolved
python/ray/tune/tests/test_searchers.py Outdated Show resolved Hide resolved
xingyousong and others added 3 commits December 10, 2024 20:27
Co-authored-by: matthewdeng <[email protected]>
Signed-off-by: xingyousong <[email protected]>
Signed-off-by: xingyousong <[email protected]>
Signed-off-by: xingyousong <[email protected]>
@xingyousong
Copy link
Author

@matthewdeng

The newest microchecks (and ReadTheDocs tests) are complaining that ray.tune.search.vizier doesn't exist. Are there any more __init__ files I need to deal with? It's weird since I already got python/ray/tune/search/vizier/__init__.py setup.

For example, the most recent microcheck:

[2024-12-23T07:02:18Z] Extension error (sphinx.ext.autosummary):
--
  | [2024-12-23T07:02:18Z] Handler <function process_generate_options at 0x7f8f9444d8b0> for event 'builder-inited' threw an exception (exception: no module named ray.tune.search.vizier)
  | [2024-12-23T07:02:18Z] make: *** [Makefile:76: html] Error 2
  | [2024-12-23T07:02:18Z] make: Leaving directory '/ray/doc'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tune Tune-related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants