mbrl-lib icon indicating copy to clipboard operation
mbrl-lib copied to clipboard

Misc updates to requirements and build files

Open Rohan138 opened this issue 1 year ago • 12 comments

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.

  1. black>=22.3.0: This is to fix import issues with click and unicodefun documented here
  2. The .pre-commit-config and setup.cfg seem to be pinning the python version for black and mypy at 3.7
  3. 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) in env.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.

Rohan138 avatar Aug 03 '22 06:08 Rohan138

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.

luisenp avatar Aug 03 '22 13:08 luisenp

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).

natolambert avatar Aug 03 '22 16:08 natolambert

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.

Rohan138 avatar Aug 03 '22 22:08 Rohan138

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.

Rohan138 avatar Aug 03 '22 22:08 Rohan138

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.

luisenp avatar Aug 05 '22 21:08 luisenp

Rebased; do you know why you need to specify python=3.9?

Rohan138 avatar Aug 09 '22 04:08 Rohan138

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?

luisenp avatar Aug 09 '22 13:08 luisenp

No, I tried running both pre-commit run --all-files and mypy mbrl --no-strict-optional --ignore-missing-imports, both passed...

Rohan138 avatar Aug 10 '22 04:08 Rohan138

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.

image

luisenp avatar Aug 10 '22 14:08 luisenp

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.

luisenp avatar Aug 10 '22 15:08 luisenp

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?

Rohan138 avatar Aug 13 '22 11:08 Rohan138

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

luisenp avatar Aug 15 '22 18:08 luisenp

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.

natolambert avatar Dec 01 '22 22:12 natolambert

Pushing some changes here https://github.com/Rohan138/mbrl-lib/pull/1 for the pr

natolambert avatar Dec 01 '22 22:12 natolambert

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.

Rohan138 avatar Dec 01 '22 23:12 Rohan138

I'd help with the changes and am a technical advisor to Farama if we have questions, I'm sure they would help.

natolambert avatar Dec 01 '22 23:12 natolambert