get_dataset and cli download ability #322
Conversation
There was a problem hiding this comment.
@grahamannett Thank you for your PR! Largely looks good to me.
Regarding the formatting, can you try this?
# formatting
pip install black isort docformatter
./scripts/format
# linter
pip install mypy pylint==2.13.5
./scripts/lint
Other than that, I'd like you to give me an example I can try an experiment with Minari?
Once again, thank you for your contribution!
d3rlpy/cli.py
Outdated
| subprocess.run(["pip3", "uninstall", "-y", "pybullet"], check=True) | ||
| else: | ||
| raise ValueError(f"Unsupported command: {name}") | ||
| match name: |
There was a problem hiding this comment.
Can you revert this change to use if-else conditions? Because match is supported since Python3.10, however d3rlpy supports Python3.8 or above. It's a little too early to use this Python feature.
There was a problem hiding this comment.
yeah figured you wouldnt want this as its gonna break all <3.10 but thought it was worth a try. its a shame it will never really be a used feature as i tend to find it pretty apt for exactly this situation
Codecov Report
@@ Coverage Diff @@
## master #322 +/- ##
==========================================
+ Coverage 92.59% 92.86% +0.27%
==========================================
Files 110 108 -2
Lines 7385 7150 -235
==========================================
- Hits 6838 6640 -198
+ Misses 547 510 -37
... and 54 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
@takuseno yeah the format stuff I just had to copy the flags from the format script into my workspace settings. still waiting on the guy i was talking to at minari/farama to version bump and then will update with version/example/test/whatever |
grahamannett
force-pushed
the
graham/feature-minari-dataset-integration
branch
from
88bc8c6 to
f108a43
Compare
|
i had to redo my branch and think this is everything updated. removed the case/match and included a test. I thought before that there was a place that you pinned d4rl/gym/etc but now i dont see it so its just in the install part. minari version 0.4.2 was just released so that it will download the dataset if its not already. let me know if you want me to fix/change anything else (or if i forgot something as i had a messed up PR) @takuseno |
|
@takuseno do you need me to do anything more? |
|
@grahamannett Thanks for the update! There seem linter errors, but I'll follow up on this. Thank you for your contribution! |
Am waiting for the group at minari to release a minor version update so I can pin the version which will allow
minari.load_dataset(env_name, download=True)to work but here is an integration for minari which is the same idea as d4rl but seems like that group want people to switch over to using minari as it is a more standardized format for offline-rl.Here is a naive implementation of getting the dataset but requires loading it all into memory which I think can be avoided (so episodes are only read from disk once asked for) once I understand a little deeper the
ReplayBufferclass. Other than that is there anything else needed for a PR? Maybe can include a test as well.Also whats up with the formatting, it seems like you use line length of 80 on the format script but the pylintrc uses 100? I may need to reformat as I was using default black line length which is 88 i believe.