Misc updates to requirements and build files

Rohan138 commented 2 years ago

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 with click and unicodefun documented here
The .pre-commit-config and setup.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) 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.

luisenp commented 2 years ago

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.

natolambert commented 2 years ago

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

Rohan138 commented 2 years ago

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 commented 2 years ago

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.

luisenp commented 2 years ago

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.

Rohan138 commented 2 years ago

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

luisenp commented 2 years ago

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?

Rohan138 commented 2 years ago

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

luisenp commented 2 years ago

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.

luisenp commented 2 years ago

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.

Rohan138 commented 2 years ago

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?

luisenp commented 2 years ago

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

natolambert commented 1 year ago

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 commented 1 year ago

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

Rohan138 commented 1 year ago

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.

natolambert commented 1 year ago

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

Rohan138 commented 1 year ago

Closing this PR since it's become stale and a little convoluted;I'm cherry-picking the neccessary changes and moving them to a separate PR.

facebookresearch / mbrl-lib