-
Notifications
You must be signed in to change notification settings - Fork 49
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
Update jax, brax and flax versions (fixes the jax.tree_util warnings) #76
Conversation
We should also update flax and chex version to fix the warnings completely - Flax (0.6.0) and chex (0.1.4) versions have these changes updated. |
That was included in the PR. However, I've just changed it a bit. Because:
Lines 27 to 29 in 8aa7235
|
…rent brax versions
Codecov Report
@@ Coverage Diff @@
## main #76 +/- ##
==========================================
- Coverage 89.49% 89.49% -0.01%
==========================================
Files 66 66
Lines 3864 3863 -1
==========================================
- Hits 3458 3457 -1
Misses 406 406
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Hey 👋 Have you made a few double checks to be sure that those updates won't affect performances? |
Hi guys, I reviewed the PR and am happy with it except for the |
Related to issue #74
This PR also adds the wrapper
CompletedEvalWrapper
that used to be in Brax, but has been removed in the most recent versions.In summary, this wrapper used to be present in Brax, and we rely on some elements of it in 4 algorithms (I think those are: SAC, DIAYN, DADS and TD3), and their tests where all failing for that reason.
So what I did is copying the old wrapper of Brax here, and now all the tests pass.
This is a provisional solution to make it work in the same way as before.
Maybe we can improve and adapt our own code to the new structure of QDax, but I think that is for another time/PR.