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

[RLLib] MAML extension for all models except RNNs #11337

Merged
merged 10 commits into from
Nov 13, 2020
Merged

[RLLib] MAML extension for all models except RNNs #11337

merged 10 commits into from
Nov 13, 2020

Conversation

michaelzhiluo
Copy link
Contributor

We now use a higher order API from Facebook for higher order gradients. This allows for extension for many different types of models for metaupdate.

Why are these changes needed?

Related issue number

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

sven1977
sven1977 previously requested changes Oct 12, 2020
View reviewed changes
Copy link
Contributor

@sven1977 sven1977 left a comment

Choose a reason for hiding this comment

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

Very cool! Thanks for fixing some of these limitations.

Please change the import-order.

rllib/agents/maml/maml_torch_policy.py Outdated Show resolved Hide resolved
@ericl
Copy link
Contributor

ericl commented Oct 19, 2020

Is this ready to merge?

@michaelzhiluo
Copy link
Contributor Author

Yes, I think so

@ericl
Copy link
Contributor

ericl commented Oct 20, 2020

Looks like maml and mbmpo tests are failing

@michaelzhiluo
Copy link
Contributor Author

Looks like higher should be an optional import...i'll fix it

@michaelzhiluo
Copy link
Contributor Author

This should work now

@ericl ericl self-assigned this Oct 22, 2020
@ericl
Copy link
Contributor

ericl commented Oct 22, 2020 

  File "/home/travis/.cache/bazel/_bazel_travis/b88c129a127452fc94033a29d9f90e20/execroot/com_github_ray_project_ray/bazel-out/k8-opt/bin/rllib/tests/test_dependency_torch.runfiles/com_github_ray_project_ray/rllib/tests/test_dependency_torch.py", line 12, in <module>
    "Torch initially present, when it shouldn't."

@michaelzhiluo I think what you need to do is move the import into the class itself instead of pulling it in at top-level. It seems like higher is importing torch automatically.

@ericl ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Oct 22, 2020
@michaelzhiluo
Copy link
Contributor Author

@ericl Makes sense; the pytorch maml tests will fail so I'll go ahead and remove that test for now

@sven1977
Copy link
Contributor

Hey, let's get this merged. Only test_dependency_torch is causing problems now. Everything else looks good.

@sven1977
Copy link
Contributor

@michaelzhiluo

@michaelzhiluo
Copy link
Contributor Author

@ericl I think it should be good to go. There are no errors for imports!

ericl
ericl reviewed Nov 9, 2020
View reviewed changes
rllib/BUILD
# tags = ["agents_dir"],
# size = "medium",
# srcs = ["agents/mbmpo/tests/test_mbmpo.py"]
#)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this test be put back now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out that MBMPO depends on Pytorch MAML, which has higher

ericl
ericl reviewed Nov 9, 2020
View reviewed changes
Copy link
Contributor

@ericl ericl left a comment

Choose a reason for hiding this comment

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

Question on the test removal

@michaelzhiluo
Copy link
Contributor Author

@ericl Added explanation why MBMPO tests are removed

@ericl ericl merged commit 59bc1e6 into ray-project:master Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sven1977 sven1977 sven1977 left review comments

@ericl ericl ericl approved these changes

Assignees

@ericl ericl

@sven1977 sven1977

Labels
@author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@michaelzhiluo @ericl @sven1977