-
Notifications
You must be signed in to change notification settings - Fork 157
Misc updates to requirements and build files #161
Conversation
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 |
Yes, currently the default is still the normal obs, reward, done, info; however done is being split into |
On mypy, that's weird, the mypy errors don't show up unless you remove the |
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 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 |
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 |
No, I tried running both |
OK, looks like all of these errors are due to |
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 |
* clean PID implementation * minor text changes * make batch friendly, add tests * lint tests * make tests deterministic * fix docstring * add colab to readme * clean PR for mbrl-lib
* fix epoch/episode iteration bug in mbpo.py * fixed trailing whitespace Co-authored-by: Matthias <ga26qen@mytum.de>
I was also looking at this one because merging the change away from black It almost feels like to get mypy to work you would need to add wrappers to all the For reference, here's |
Pushing some changes here Rohan138#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. |
Fixes for tooling improvement branch
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. |
Types of changes
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.pre-commit-config
andsetup.cfg
seem to be pinning the python version for black and mypy at 3.7gym
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