mbrl-lib
mbrl-lib copied to clipboard
Misc updates to requirements and build files
Types of changes
- [X] Docs change / refactoring / dependency upgrade
- [X] Bug fix (non-breaking change which fixes an issue)
- [X] New feature (non-breaking change which adds functionality)
- [X] Breaking change (fix or feature that would cause existing functionality to change)
Motivation and Context / Related issue
Move the minor changes from #151 to here.
-
black>=22.3.0
: This is to fix import issues withclick
andunicodefun
documented here - The
.pre-commit-config
andsetup.cfg
seem to be pinning the python version for black and mypy at 3.7 -
gym
has undergone significant maintance over the past year, and 0.17.2 is basically deprecated. However,0.25.0
soft-migrates to returning five values (instead of four) inenv.step
, along with a few other breaking changes.
How Has This Been Tested (if it applies)
I created a clean conda env with python=3.8.10
, built mbrl-lib, ran the tests, and ran pre-commit.
Checklist
- [X] The documentation is up-to-date with the changes I made.
- [X] I have read the CONTRIBUTING document and completed the CLA (see CONTRIBUTING).
- [X] All tests passed, and additional code has been covered with new tests.
Thanks a lot for the update @Rohan138! Looks like we will have to fix a bunch of mypy errors before we can merge. If you can try to address some of them that would be great, but I'll also set aside Friday to help out with this.
I'm here to say that I'm shocked that they changes the env.step()
API. A bit of a weird one, I wonder if there's a kwarg
for keeping it the same, so everyone can env.step(action, use_old_api=True)
.
Yes, currently the default is still the normal obs, reward, done, info; however done is being split into terminated
and truncated
over the next few releases. There's a few other changes e.g. env.seed
and env.render
are being deprecated in favor of env.reset(seed)
and __init__(render_mode)
. This issue summarizes most of the changes, but they'll take some effort to migrate to.
On mypy, that's weird, the mypy errors don't show up unless you remove the python=3.7
pin. I'll try to retest with fresh conda envs for all four python versions.
Fixed the mypy errors in #163. There was some inconsistency between what we got from pre-commit's mypy command and directly running from the command line. Setting python_version=3.9
got rid of this, and then I fixed a few type errors.
Can you rebase on top of latest main and push? With the latest version we only need to keep the changes to requirements file in this PR.
Rebased; do you know why you need to specify python=3.9
?
I'm not sure why the 3.9 is needed, to be honest. Also, don't really understand why CI is failing in this branch and not in main; I guess it might be related to gym's version. If you set python_version=3.9
and run pre-commit
locally, do you get these errors?
No, I tried running both pre-commit run --all-files
and mypy mbrl --no-strict-optional --ignore-missing-imports
, both passed...
Weird, I get some errors when I run. Maybe a python version issue? In any case, I also get these errors in main
, so I don't really know how the previous merge failed to detect them, but I'll try to fix on my side.

OK, looks like all of these errors are due to gym
's version. If I downgrade my environment's gym
to 0.17.2 mypy passes. Also, some of these changes look breaking, so maybe we want to hold off on this PR until we can ensure all utilities are compatible with 0.24.
Hmm, that works for now, but 0.17.2 is a little old and out of date. We should probably bump the version past 0.19.0 at the very least (That's when Gym became actively maintained)
Also, I still can't replicate the mypy errors locally, any ideas on your setup?
I'm definitely interested in bumping the version to 0.24, it's mostly a matter of me finding some time to do it, which is a bit hard these days :(
I'm running mypy mbrl --no-strict-optional --ignore-missing-imports
and the conda environment produces the conda list
output in the attached text file.
mbrl_conda.txt
I was also looking at this one because merging the change away from black python 3.7
would be super useful. It's definitely pretty weird. The Env[Any, Any]
is really weird, and then a lot of places where env.reset()
is called, the type checker is seeing Union[Any, Tuple[Any, Dict[Any, Any]]]
instead of the ndarray
.
It almost feels like to get mypy to work you would need to add wrappers to all the Env
classes to define the attributes. Maybe a MBRLGymEnv or something.
For reference, here's gym==0.24.1
environment. There's an ObsType
variable, which is messing up all the expected outputs of step() / reset() and the replaybuffers.
Pushing some changes here https://github.com/Rohan138/mbrl-lib/pull/1 for the pr
Running the tests now; it might also be feasible to update all the way to gym==0.26 or Gymnasium (the new fork of gym by the maintainers; gym is now unmaintained). They have better type support and fixes, but some large API changes to the step and reset methods.
I'd help with the changes and am a technical advisor to Farama if we have questions, I'm sure they would help.