search

IntelLabs / coach

Reinforcement Learning Coach by Intel AI Lab enables easy experimentation with state of the art Reinforcement Learning algorithms
https://intellabs.github.io/coach/
Apache License 2.0
2.32k stars 459 forks source link

Mujoco build is failing #264

Closed gal-leibovich closed 5 years ago

gal-leibovich commented 5 years ago

e.g. in here - https://circleci.com/gh/NervanaSystems/coach/tree/pull%2F263 @anabwan @scttl could you please take a look?

anabwan commented 5 years ago

Mujoco issue: https://github.com/openai/mujoco-py/issues/374

I tried to run it manually on my machine and got same error

gal-leibovich commented 5 years ago

Why do you think that it is the same issue as in the CI? The reported error seems to be numpy related, PTAL here.

anabwan commented 5 years ago

you probably right, I tried to uninstall/ install numpy with different versions in my machine, but still get error in mujoco-py installation, since I don't have a license (seems like the numpy issue not happen).

I only thought about "--no-cache" solution for the pip install or we can uninstall && install numpy manually..

let's wait for @scttl comment

scttl commented 5 years ago

Ok took a look and the issue isn't the numpy error -- that's always been there and is based on mujoco-py whl building depending on numpy but it hasn't yet been installed, it still builds the whl ok and numpy is installed later.

The actual issue is that we aren't pinning the mujoco-py version in the Dockerfile (so we pull latest), and on Friday mujoco-py pushed a new release that supports mujoco-2.0 by default: https://github.com/openai/mujoco-py/releases

So we have two options for fix:

  1. update the Dockerfile to install mujoco-2.0 instead of mujoco-1.5 that we currently install
  2. pin mujoco-py to the latest mujoco-1.5 compatible release.

I'm going to put option 2. in place for now so we can get back to building in a known validated state but we should look at upgrading to mujoco-2.0 soon. Here's the changelist: https://www.roboti.us/index.html#mujoco200 do you think that would break anything in coach @galleibo-intel?

scttl commented 5 years ago

I added the fix in the dockerfiles branch: https://github.com/NervanaSystems/coach/pull/169/commits/cf4ecb665ffe5127d8dd122230feb4e785ea21cd

and manually pushed these updated Dockerfiles to the registry for now so future test runs will pick up these changes while we await merging #169

gal-leibovich commented 5 years ago

Thanks @scttl!

In the past, I remember new mujoco versions degrading some presets' convergence, due to small changes in the environment behavior. According to the changelist, it doesn't sound like this is the case, but I think we actually would want to thoroughly test Mujoco 2.0, with our current algorithms/presets, before officially changing to it.

I agree that option #2 is the way to go for now.

scttl commented 5 years ago

confirmed building ok in CI now via option 2. and Gal opened a ticket to track the Mujoco 2.0 upgrade on #266