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.

Sign up for GitHub

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

Jump to bottom

Update ray example to reflect Hydra 1.1 changes #1490

Merged
merged 1 commit into from
Apr 29, 2021
Merged

Update ray example to reflect Hydra 1.1 changes #1490

merged 1 commit into from
Apr 29, 2021

Conversation

jieru-hu
Copy link
Contributor

@jieru-hu jieru-hu commented Mar 24, 2021 •
edited
Loading

Closes #1471
Verified the example apps run as expected.

@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 Mar 24, 2021
omry
omry reviewed Mar 24, 2021
View reviewed changes
website/docs/plugins/ray_launcher.md Outdated
@@ -144,12 +144,59 @@ $ python my_app.py --multirun task=1,2,3
</details>


<details><summary>Upload & Download from remote cluster</summary>
##### Overriding default `ray_aws` config
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need generic explanation page to explain how to configure plugins in general in Hydra 1.1 (that all plugins can refer to from their docs).
It doesn't make sense for each plugin doc page to have to explain the same thing.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I will create a new page for configuring plugins.

@omry
Copy link
Collaborator

omry commented Mar 25, 2021

#1471
Verified the example apps run as expected.

You want Fixes #1471 or Closes #1471.

@jieru-hu
Copy link
Contributor Author

#1471
Verified the example apps run as expected.

You want Fixes #1471 or Closes #1471.

updated the description :)

@jieru-hu jieru-hu marked this pull request as draft March 25, 2021 17:29
@jieru-hu jieru-hu mentioned this pull request Mar 26, 2021
Merged
@jieru-hu
Copy link
Contributor Author

I've created #1506 to address the comment, once that one lands, I will fix this PR accordingly.

@omry
Copy link
Collaborator

omry commented Mar 26, 2021

sounds good.

@jieru-hu
Copy link
Contributor Author

with #1506 merged, it makes sense for this PR to only modify the example apps

for doc changes, all plugin doc will be updated in #1582

@jieru-hu jieru-hu changed the title Update ray example and doc to reflect Hydra 1.1 changes Update ray example to reflect Hydra 1.1 changes Apr 29, 2021
@jieru-hu jieru-hu requested a review from omry April 29, 2021 17:50
@jieru-hu jieru-hu marked this pull request as ready for review April 29, 2021 17:50
@omry
Copy link
Collaborator

omry commented Apr 29, 2021

with #1506 merged, it makes sense for this PR to only modify the example apps

for doc changes, all plugin doc will be updated in #1582

Since it looks like for each plugin, we will want to update both the example and the documentation, it could be easier to complete the Ray changes in this PR, including the docs.

omry
omry approved these changes Apr 29, 2021
View reviewed changes
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.

looking good.
consider finishing the doc changes for this plugin.

@jieru-hu
Copy link
Contributor Author

looking good.
consider finishing the doc changes for this plugin.

will do!

@jieru-hu jieru-hu merged commit 88152f1 into facebookresearch:master Apr 29, 2021
@jieru-hu jieru-hu deleted the ray_1.1 branch April 29, 2021 22:18
@omry
Copy link
Collaborator

omry commented Apr 29, 2021

looking good.
consider finishing the doc changes for this plugin.

will do!

I meant in this PR :).
never mind, a different PR is fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@omry omry omry approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] Ray plugin ConfigStore schema need to be renamed
3 participants
@jieru-hu @omry @facebook-github-bot