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

Add Ax plugin for Hydra #321

Closed
wants to merge 86 commits into from
Closed

Add Ax plugin for Hydra #321

wants to merge 86 commits into from

Conversation

shagunsodhani
Copy link
Contributor

Motivation

Hydra Plugin to interface with Adaptive Experimentation Platform

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

To be added

Related Issues and PRs

#291

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 10, 2019
pip install hydra-ax
```

We include an example of how to use this plugin. The file `plugins/hydra_ax/example/banana.py` implements the [Rosenbrock function (aka Banana function)](https://en.wikipedia.org/wiki/Rosenbrock_function). The return value of the function should be the value that we want to optimize.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remember to link this to the actual file in GitHub once this lands.

plugins/hydra_ax/README.md Outdated Show resolved Hide resolved
plugins/hydra_ax/README.md Outdated Show resolved Hide resolved
plugins/hydra_ax/example/banana.py Outdated Show resolved Hide resolved
Trial = NewType("Trial", List[str])

# TODO: output directory is overwriting, job.num should be adjusted (depends on issue #284)
# TODO: Support running multiple random seeds, aggregate mean and SEM
Copy link
Collaborator

Choose a reason for hiding this comment

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

While chatting with the Ax team they suggested that maybe Ax would do a better job at this automatically. not sure I am convinced but worth discussing with them.

plugins/hydra_ax/hydra_plugins/ax_sweeper/ax_sweeper.py Outdated Show resolved Hide resolved
plugins/hydra_ax/hydra_plugins/ax_sweeper/ax_sweeper.py Outdated Show resolved Hide resolved
plugins/hydra_ax/setup.py Outdated Show resolved Hide resolved
plugins/hydra_ax/tests/test_example_sweeper_plugin.py Outdated Show resolved Hide resolved
@omry omry mentioned this pull request Dec 29, 2019
@omry
Copy link
Collaborator

omry commented Dec 30, 2019

The next major version of Hydra will break compatibility (dropping Python <= 3.5, switching to OmegaConf 2.0 etc).
I may be a months or two before this version is ready.
The 0.11_branch will be the release branch for the current 0.11 release line.
in the mean time I suggest that you continue your development on top of the 0.11_branch to ensure it's working for people using the stable version.

@omry
Copy link
Collaborator

omry commented Dec 30, 2019

You can also just ignore this and continue working against master, testing at the end that the plugin actually works against 0.11 as well (I suspect it will).

@shagunsodhani
Copy link
Contributor Author

@omry yeah I think I have most of the pieces in place now :)

@omry
Copy link
Collaborator

omry commented Dec 30, 2019

FYI:
I just landed the OmegaConf 2.0 upgrade, and I am going to start cleaning up Python 2.7 left overs now, as well as annotate the code and enable mypy.

@omry
Copy link
Collaborator

omry commented Jan 6, 2020

Master is now on OmegaConf 2.0 and Python >= 3.6.
New requirements are that all code passes mypy --strict . and isort for import sorting.
When you merge in, expect to spend some time type annotating the new code.

The name of the module was different from the name of the plugin. This
caused some tests to fail.
@omry
Copy link
Collaborator

omry commented Jan 12, 2020

no need to force push normally. you can just keep piling up the diffs. when it's all ready I can squash it at commit or you can have one last force push to clean things up.

Force pushing makes it hard for others to interact with your diff and hard for me to see what changed since the last time.

@shagunsodhani
Copy link
Contributor Author

no need to force push normally. you can just keep piling up the diffs. when it's all ready I can squash it at commit or you can have one last force push to clean things up.

Force pushing makes it hard for others to interact with your diff and hard for me to see what changed since the last time.

Will take care :)

@omry
Copy link
Collaborator

omry commented Jan 15, 2020

Heads up:
There is some API cleanup in progress that will effect plugins.
See the examples plugins for updated examples. if you run into issues rebasing let me know.

@omry
Copy link
Collaborator

omry commented Jan 25, 2020

Let's try to finish this?
What's left before we can land this to master?

@shagunsodhani
Copy link
Contributor Author

Fixing the isort check. I fixed the other issues/comments.

@omry
Copy link
Collaborator

omry commented Jan 26, 2020

So we have two issues:
the lint issue on all systems, and the dependency problem on Windows.

Copy link
Collaborator

@omry omry left a comment

Choose a reason for hiding this comment

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

Getting there.

plugins/hydra_ax_sweeper/README.md Show resolved Hide resolved
plugins/hydra_ax_sweeper/README.md Outdated Show resolved Hide resolved
plugins/hydra_ax_sweeper/setup.py Outdated Show resolved Hide resolved
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 15, 2020

This pull request introduces 1 alert when merging 144e453 into af78b06 - view on LGTM.com

new alerts:

  • 1 for Module imports itself

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 16, 2020

This pull request introduces 1 alert when merging c08838d into df0b6a1 - view on LGTM.com

new alerts:

  • 1 for Module imports itself

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 17, 2020

This pull request introduces 2 alerts when merging 77e037d into df0b6a1 - view on LGTM.com

new alerts:

  • 1 for Module imports itself
  • 1 for Variable defined multiple times

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 17, 2020

This pull request introduces 2 alerts when merging ece1aa5 into df0b6a1 - view on LGTM.com

new alerts:

  • 1 for Module imports itself
  • 1 for Variable defined multiple times

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 17, 2020

This pull request introduces 2 alerts when merging dbecffc into df0b6a1 - view on LGTM.com

new alerts:

  • 1 for Module imports itself
  • 1 for Variable defined multiple times

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 17, 2020

This pull request introduces 2 alerts when merging b961968 into df0b6a1 - view on LGTM.com

new alerts:

  • 1 for Module imports itself
  • 1 for Variable defined multiple times

hydra/test_utils/launcher_common_tests.py Outdated Show resolved Hide resolved
hydra/test_utils/test_utils.py Outdated Show resolved Hide resolved
plugins/hydra_ax_sweeper/example/conf/config.yaml Outdated Show resolved Hide resolved
plugins/hydra_ax_sweeper/tests/banana.py Outdated Show resolved Hide resolved
plugins/hydra_ax_sweeper/tests/config/banana.yaml Outdated Show resolved Hide resolved
plugins/hydra_ax_sweeper/tests/test_ax_sweeper_plugin.py Outdated Show resolved Hide resolved
plugins/hydra_ax_sweeper/tests/test_ax_sweeper_plugin.py Outdated Show resolved Hide resolved
plugins/hydra_ax_sweeper/tests/test_ax_sweeper_plugin.py Outdated Show resolved Hide resolved
plugins/hydra_ax_sweeper/tests/test_ax_sweeper_plugin.py Outdated Show resolved Hide resolved
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 17, 2020

This pull request introduces 2 alerts when merging b56e4dd into df0b6a1 - view on LGTM.com

new alerts:

  • 1 for Module imports itself
  • 1 for Variable defined multiple times

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 20, 2020

This pull request introduces 2 alerts when merging 646aac9 into 45ba5e3 - view on LGTM.com

new alerts:

  • 1 for Module imports itself
  • 1 for Variable defined multiple times

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 20, 2020

This pull request introduces 2 alerts when merging 89d31cc into 5f8d689 - view on LGTM.com

new alerts:

  • 1 for Module imports itself
  • 1 for Variable defined multiple times

return z


def test_launched_jobs(sweep_runner: TSweepRunner,) -> None: # noqa: F811
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add additional test covering:

  1. command line only usage (no config).
  2. mixed command line and config usage.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 20, 2020

This pull request introduces 1 alert when merging b9148d1 into 4106845 - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 20, 2020

This pull request introduces 1 alert when merging a0546d2 into 4106845 - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

@omry
Copy link
Collaborator

omry commented Feb 22, 2020

let's close this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants