-
Notifications
You must be signed in to change notification settings - Fork 258
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
Replace imitation environments with seals #541
Conversation
Note will need to remove mentions of |
Codecov Report
@@ Coverage Diff @@
## master #541 +/- ##
==========================================
+ Coverage 97.45% 97.48% +0.03%
==========================================
Files 88 83 -5
Lines 8401 8079 -322
==========================================
- Hits 8187 7876 -311
+ Misses 214 203 -11
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Tests are passing now. The errors were quite silly:
I think that the seals pull (HumanCompatibleAI/seals#58) should be good to merge, except perhaps for adding some tests. @AdamGleave do you have any suggestions as to what tests we could add? Once we merge that, we replace the version pointer on this PR and we can merge this too. |
…imitation-envs-to-seals
…imitation-envs-to-seals
…imitation-envs-to-seals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks fine -- satisfying how much code we were able to delete :) Definitely was the right move to migrate to seals
.
I'm not sure about the from seals import base_envs as envs
, OTOH would prefer it to just be from seals import base_envs
as envs
seems sufficiently generic it could easily cause clashes down the line and is another name to keep track of (what's envs
? oh, it's seals.base_envs
) but I'm happy to hear arguments to the contrary.
Other suggestion is to beef up the seals testing with some of the code removed in this PR. Obviously not something to resolve in this PR, but I'd feel better if we either decide against that or open an issue/PR in seals to track that before we close this PR and forget about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Fixes #501. (Depends on HumanCompatibleAI/seals#58).