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

Supporting Hydra Experiments #3

Closed
captain-pool opened this issue Feb 25, 2022 · 12 comments
Closed

Supporting Hydra Experiments #3

captain-pool opened this issue Feb 25, 2022 · 12 comments
Labels
enhancement New feature or request

Comments

@captain-pool
Copy link
Owner

Currently, the sweeper plugin can be used to override parameter values directly. However, it would be nice to have a way to sweep over hydra experiments.

@captain-pool captain-pool added the enhancement New feature or request label Feb 25, 2022
@tesfaldet
Copy link
Contributor

I was just going to ask for this! I'm actually modifying the code myself to support this functionality (you probably noticed my fork), so if I get it working maybe I can make a PR?

@captain-pool
Copy link
Owner Author

Sure! Please feel free. 😃 Thanks again for helping out!

As a pointer, it might be helpful for you to look into the Hydra Launcher API, since I found loading experimental config files, occur inside the launcher, and the sweeper API only passes the name of the experiment file to load. So for example, if I want to sweep over: +experiment=expt1, expt2, sweeper will pass the launcher a list like this: ['+experiment=expt1', '+experiment=expt2'].

Another constraint is wandb.config doesn't allow any parameter with special character in the name. So, we can't directly add +experiment as a parameter to wandb.config and poll it as a parameter from the agent.

I however believe there must be some simple work around that allows logging these experimental configs along with loading them and invoking the task_function.

@tesfaldet
Copy link
Contributor

Yup, I'm quite familiar with the Launcher API at this point. I've been deep into it over the past week because of this issue. I'm about halfway through a solution at the moment.

@tesfaldet
Copy link
Contributor

I've been using the Nevergrad sweeper as a good comparison to go by since I've been using it a lot myself. Specifically, it deals with params coming through overrides as well as through the config file. So I'm trying to make it so that you pass a params MutableMapping via hydra.sweep.params with the params you want to be sweeped and pass that to the sweep dict to wandb.sweep. I'm also trying to follow this PR as guidance because of this change for the next Hydra update.

@tesfaldet
Copy link
Contributor

Anyways you'll see when I make the PR! I just need to have something working first :) Almost there!

@captain-pool
Copy link
Owner Author

This looks very exciting! Looking forward for the PR. Also, when you send it, please install the pre-commit hook and lint the code. You might want to skip pylint checker initially (using $ SKIP=pylint git commit ...) as I skipped adding docstring to get this plugin out fast. But I'm currently working on writing them and adding some documentation to pass the pylint tests.

@tesfaldet
Copy link
Contributor

PR started here #5

@tesfaldet
Copy link
Contributor

PR finished 👍

@captain-pool
Copy link
Owner Author

Amazing! Thank you for your contribution @tesfaldet! From a very high level, it looks really good. I'll be OOO this week. Will be able to provide a detailed review early next week.

@tesfaldet
Copy link
Contributor

Wonderful! I apologize in advance for the ugly monkeypatching but it was necessary, unfortunately. Hopefully we can add some unit tests soon!

I also have questions regarding the pre-commit hooks. Namely the yaml formatting which seems to prefer 4-space indentations when the recommend amount is 2 spaces. This is also the number of spaces used in the yaml files within Hydra, so if this plugin were to be PR'd in the Hydra repo then I'm sure it'll fail that check.

@tesfaldet
Copy link
Contributor

I believe this can be closed due to #5

@captain-pool
Copy link
Owner Author

Yes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants