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

Make dm_control an extra dependency #828

Merged
merged 1 commit into from
Aug 19, 2019
Merged

Conversation

krzentner
Copy link
Contributor

Because dm_control needs to be installed from vcs or tarball, garage
cannot be directly installed in pipenv (see this
issue
).

I've copied the example that used our dm_control wrapper, modified
the original example to use gym, and modified the copy to clarify the
extra dependency.

I've also modified the garage.env.dm_control module to report a
helpful error message if dm_control isn't found.

pip install garage[all] will still install dm_control.

Because dm_control needs to be installed from vcs or tarball, garage
cannot be directly installed in pipenv (see [this
issue](pypa/pipenv#3396)).

I've copied the example that used our dm_control wrapper, modified
the original example to use gym, and modified the copy to clarify the
extra dependency.

I've also modified the `garage.env.dm_control` module to report a
helpful error message if dm_control isn't found.

`pip install garage[all]` will still install dm_control.
@krzentner krzentner requested a review from a team as a code owner August 1, 2019 03:55
@codecov
Copy link

codecov bot commented Aug 1, 2019

Codecov Report

Merging #828 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #828      +/-   ##
==========================================
- Coverage   73.48%   73.44%   -0.04%     
==========================================
  Files         178      178              
  Lines       10295    10295              
  Branches     1304     1304              
==========================================
- Hits         7565     7561       -4     
- Misses       2365     2368       +3     
- Partials      365      366       +1
Impacted Files Coverage Δ
src/garage/misc/special.py 30.3% <0%> (-3.04%) ⬇️
src/garage/tf/optimizers/first_order_optimizer.py 65.27% <0%> (-2.78%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4a6271...d0a2877. Read the comment docs.

@ryanjulian
Copy link
Member

It can be installed with pipenv, but requires the --pre flag. pipenv refuses to match dependencies <1.0 by default.

If this is the only package which requires the flag, then I'm happy to move it into an optional. If the mainline packages require --pre anyway, then I want to avoid running in circles around the issue.

Copy link
Member

@zhanpenghe zhanpenghe left a comment

Choose a reason for hiding this comment

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

This looks great!

@krzentner
Copy link
Contributor Author

Ah, I see. This is indeed the only package which requires --pre. As far as I can tell, versions before 1.0 are still considered release versions, but versions like e.g. 2.0rc1 aren't.

@gitanshu
Copy link
Contributor

gitanshu commented Aug 5, 2019

I don't need to use --pre for anything with pipenv if I install pipenv from master. OTOH, if I use the last release of pipenv (2018.11.26), even --pre doesn't work for dm_control.

@krzentner
Copy link
Contributor Author

I don't need to use --pre for anything with pipenv if I install pipenv from master. OTOH, if I use the last release of pipenv (2018.11.26), even --pre doesn't work for dm_control.

Ah, thanks for testing this. I just know that I couldn't get it to work with the latest release. If a pipenv release fixes this soon, then I'd be happy to drop this change.

@ryanjulian
Copy link
Member

I think the best move is probably to advise people to use pre-release pipenv. I don't see anything on their GitHub indicating a release is imminent, but working around this is kind of a PITA for a normal user.

ref: pypa/pipenv#2596

@gitanshu
Copy link
Contributor

I opened a PR for that #839

@krzentner
Copy link
Contributor Author

After experimenting with pre-release pipenv for a bit, I don't think advising people to use it is a good idea. It (and prerelease pip) break sometimes, and are incredibly difficult to fix afterwards.

@ryanjulian
Copy link
Member

Okay, let's roll forward with this fix then.

@krzentner
Copy link
Contributor Author

Okay, I'll merge this then. It's worth mentioning that all of garage's functionality can still be used in pipenv by directly installing the required dm_control version. This works since pipenv (at least since version 2018.11) supports installing packages from url, it just doesn't support dependencies of those packages being url / version control.

@krzentner krzentner merged commit c816d58 into master Aug 19, 2019
@krzentner krzentner deleted the make-dm_control-optional branch August 19, 2019 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants