-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[workspace] Upgrade stable_baselines3_internal to latest release v1.8.0 #19187
[workspace] Upgrade stable_baselines3_internal to latest release v1.8.0 #19187
Conversation
+@ggould-tri to finish the patch adjustment changes, please. You can push fixes to this branch. |
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.
Okay, looking into this soon-ish.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: LGTM missing from assignee ggould-tri(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @mwoehlke-kitware)
@ggould-tri any progress? |
Here is the meta-commentary:
As such, I'm going to try to do something minimal for this PR, in the not-especially-sure-and-certain faith that it's all about to change anyway. The new API is unambiguously superior and we will likely want to migrate to it promptly. |
If there is any difficulty, another option is to set drake/tools/workspace/github.bzl Lines 29 to 31 in 8c106eb
|
connection.patch no longer needed with this version (was fixed upstream)
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.
Updated to text that worked for me locally. I had some trouble getting the push to branch to work, so we'll see if I got it right...
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee ggould-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @ggould-tri and @mwoehlke-kitware)
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.
Passes CI. -(status: do not merge)
Reviewable status: LGTM missing from assignee ggould-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @ggould-tri and @mwoehlke-kitware)
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.
Was this waiting on me? +(status: single reviewer ok) if that's what's needed.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @mwoehlke-kitware)
Yeah, it was waiting for platform review to be assigned, I guess. It looks good to me, also. Feel free to squash and merge. |
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.
+(status: squashing now)
Reviewable status: complete! all discussions resolved, LGTM from assignee ggould-tri(platform)
Towards #19137.
Our patches no longer apply and need to be updated.
This change is