Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove MuJoCo dependency from SQIL notebook #800

Merged
merged 3 commits into from Oct 5, 2023

Conversation

AdamGleave
Copy link
Member

Description

Fixes #798.

WIP: @ernestum can you make a HuggingFace dataset ppo-Pendulum-v1 out of the same-name model? I think that will be enough to make this notebook work. I'm not sure how you generated datasets so deferring to you on this.

@ernestum
Copy link
Collaborator

ernestum commented Oct 4, 2023

Pushed it here. Let's see what it does ...

@ernestum
Copy link
Collaborator

ernestum commented Oct 4, 2023

Seems like the SQIL notebook tests pass (even though other tests still fail due to issues addressed in other PRs.

Copy link
Member

@Rocamonde Rocamonde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks fine to me, even though I'm missing context so I cannot speak for the high-level intent of whether replacing it with the Pendulum is the right choice. Only thing seems to be that you're no longer using seals? Is this intentional?

@ernestum
Copy link
Collaborator

ernestum commented Oct 4, 2023

The context is in the linked issue #798. Basically we just wanted to get rid of the MuJoCo dependency in the test pipeline. So we just replaced the environment with anything basic that is not MuJoCO.

@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

Merging #800 (e14c27a) into master (5b0b531) will decrease coverage by 0.05%.
Report is 11 commits behind head on master.
The diff coverage is 96.69%.

@@            Coverage Diff             @@
##           master     #800      +/-   ##
==========================================
- Coverage   96.38%   96.33%   -0.05%     
==========================================
  Files          95       98       +3     
  Lines        8901     9177     +276     
==========================================
+ Hits         8579     8841     +262     
- Misses        322      336      +14     
Files Coverage Δ
src/imitation/algorithms/adversarial/common.py 96.84% <100.00%> (+0.01%) ⬆️
src/imitation/algorithms/bc.py 98.31% <100.00%> (-0.02%) ⬇️
src/imitation/algorithms/dagger.py 100.00% <100.00%> (ø)
src/imitation/algorithms/mce_irl.py 96.17% <100.00%> (ø)
src/imitation/algorithms/sqil.py 100.00% <100.00%> (ø)
src/imitation/data/huggingface_utils.py 100.00% <ø> (ø)
src/imitation/data/wrappers.py 100.00% <100.00%> (ø)
src/imitation/policies/interactive.py 100.00% <100.00%> (ø)
src/imitation/policies/replay_buffer_wrapper.py 100.00% <100.00%> (ø)
src/imitation/rewards/reward_nets.py 100.00% <100.00%> (ø)
... and 33 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@AdamGleave AdamGleave merged commit 573b086 into master Oct 5, 2023
12 of 15 checks passed
@AdamGleave AdamGleave deleted the AdamGleave/remove-mujoco-dependency branch October 5, 2023 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update 8a_train_sqil_sac.ipynb to remove MuJoCo dependency
3 participants