[drake_gym] Add info_handler callback #21900

samzapo · 2024-09-12T16:58:08Z

This change is

jwnimmer-tri · 2024-09-16T12:58:27Z

I am not very familiar with Gym, so let me ask a few questions and then also invite others to comment.

(1) Is the info["timestep"] a convention in the Gym community? If yes, they we should add a comment in the code with cross-reference(s) to existing practice, in which case my other questions are moot.

(2) If not, then for users who want this kind of metadata, why would the following not be a better solution (in their own code, with no changes to Drake)?

observation, reward, terminated, truncated, info = env.step(action)
info["timestamp"] = env.simulator.get_context().get_time()

With this technique, users can add any metadata they like. Perhaps the problem is that the user is not directly calling env.step, because it's called from a framework? In that case ...

(3) If we do need to populate info during DrakeGymEnv.step and neither option 2 nor subclassing is a viable solution, possibly we should following the existing pattern of self.reward and add a self.info constructor argument, so that the user can pass into the constructor any kind of info-callback they like. Certainly the timestamp won't be the only kind of info ever requested / useful, so hard-coding it be exactly one thing seems unwise? Flexibility seems like the better approach, at first glance.

+@JoseBarreiros-TRI for discussion and feature review, please.

JoseBarreiros-TRI · 2024-09-16T17:26:51Z

Here are my thoughts:

afaik, info["timestep"] is not a convention in the Gymnasium environment. The only widely use info key is is_success which is often use by third-party libraries (e.g. wandb, sb3) to log success rate.
The user can perfectly do what is suggested here. Indeed, a common practice (and what is recommended by Gymnaisum) when adding more operations/info to the environment is using a wrapper.
If we want to add a sugar method to add data to info, then Add a "timestamp" item to the info return value from DrakeGymEnv.step(...) #21899 seems to be the way to go.
In summary, implementing (3) or leaving the code as it is are both satisfactory.

samzapo · 2024-09-16T17:44:46Z

@jwnimmer-tri This is a great suggestion and works, but I'd prefer to support multiple gym environments, some not using Drake (e.g.: huggingface/gym-aloha).

Since it is not a convention to provide time data in info I'd lean toward requesting an optional "info handler" DrakeGymEnv construction parameter, mentioned here:
#21899 (comment)
rather than wrapping DrakeGymEnv and overriding step(), as I've done here: samzapo/gym-drake-lca as stop-gap.

If this PR is dismissed, as the desired behavior is possible with the current setup (thanks @JoseBarreiros-TRI ). That's fine too.

jwnimmer-tri · 2024-09-16T17:54:53Z

... I'd lean toward requesting an optional "info handler" ...

Ah, thanks for the pointer; I'd missed that detail.

It sounds like everyone is happy with the "info handler" proposal, so that's probably the way to proceed.

…step_info

jwnimmer-tri

@samzapo Jenkins is showing linter errors. You can run the linters locally via bazel test --config=lint //bindings/pydrake/....

@JoseBarreiros-TRI do you have time/interest in serving as the reviewer here? If not, I can recruit someone else.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee JoseBarreiros-TRI, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @samzapo)

bindings/pydrake/gym/_drake_gym_env.py line 256 at r2 (raw file):

            terminated = False
            reward = 0
            info = dict() if self.info_handler is None else self.info_handler(self.simulator)

nit Instead of copy-pasting the if-None-else logic multiple times, we can probably do something less brittle, e.g.

(1) Don't allow the info handler be None; instead, change the default value in the constructor args to bedict (i.e., the callable constructor that does what we want as a default).

(2) Add a private helper method def _info(self): on DrakeGymEnv with the None-handling logic, and change this to info = self._info() or similar.

…step_info

JoseBarreiros-TRI

happy to review this

Reviewable status: 1 unresolved discussion, LGTM missing from assignee JoseBarreiros-TRI, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @samzapo)

samzapo

Reviewable status: LGTM missing from assignee JoseBarreiros-TRI, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @samzapo)

bindings/pydrake/gym/_drake_gym_env.py line 256 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Instead of copy-pasting the if-None-else logic multiple times, we can probably do something less brittle, e.g.

(1) Don't allow the info handler be None; instead, change the default value in the constructor args to bedict (i.e., the callable constructor that does what we want as a default).

(2) Add a private helper method def _info(self): on DrakeGymEnv with the None-handling logic, and change this to info = self._info() or similar.

Done. I've chosen to do (1), see assignment to self.info_handler logic

samzapo · 2024-09-18T13:53:56Z

bindings/pydrake/gym/_drake_gym_env.py line 308 at r3 (raw file):

        # port.
        observations = self.observation_port.Eval(context)
        info = dict() if self.info_handler is None else self.info_handler(self.simulator)

Fixing this in a moment. Immediately validating why the complicated logic on each line is hard to maintain.

Code quote:

info = dict() if self.info_handler is None else self.info_handler(self.simulator)

jwnimmer-tri

Reviewable status: 1 unresolved discussion, LGTM missing from assignee JoseBarreiros-TRI, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @samzapo)

bindings/pydrake/gym/_drake_gym_env.py line 256 at r2 (raw file):

Previously, samzapo (Samuel Zapolsky) wrote…

Done. I've chosen to do (1), see assignment to self.info_handler logic

BTW I expect that foo = lambda _: dict() can be shortened to just foo = dict. The constructor is already a callable.

samzapo

Reviewable status: LGTM missing from assignee JoseBarreiros-TRI, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @samzapo)

bindings/pydrake/gym/_drake_gym_env.py line 256 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW I expect that foo = lambda _: dict() can be shortened to just foo = dict. The constructor is already a callable.

Would this work if self.info_handler(simulator) is called (i.e., with a parameter)?

jwnimmer-tri

Reviewable status: LGTM missing from assignee JoseBarreiros-TRI, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @samzapo)

bindings/pydrake/gym/_drake_gym_env.py line 256 at r2 (raw file):

Previously, samzapo (Samuel Zapolsky) wrote…

Would this work if self.info_handler(simulator) is called (i.e., with a parameter)?

Oops, nevermind. I forgot the callback took argument.

JoseBarreiros-TRI

Reviewable status: 2 unresolved discussions, LGTM missing from assignee JoseBarreiros-TRI, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @samzapo)

bindings/pydrake/examples/gym/envs/cart_pole.py line 418 at r4 (raw file):

        observation_port_id="observations",
        reset_handler=reset_handler,
        info_handler=lambda _: dict(),

why not set this to None or better provide a nice example of an info handler

bindings/pydrake/gym/_drake_gym_env.py line 257 at r4 (raw file):

            terminated = False
            reward = 0
            info = self.info_handler(self.simulator)

I'm not sure about the expected behavior here. I think that if the run gets to this point it means that the simulator faulted which would cause an unexpected behavior when called by the info_handler(). Consider setting the info to an empty dict or info["sim_runtime_error"]=True.

JoseBarreiros-TRI

@samzapo I'm happy to help land this PR, let me know if you have any questions.

Reviewable status: 2 unresolved discussions, LGTM missing from assignee JoseBarreiros-TRI, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @samzapo)

samzapo

Hi @JoseBarreiros-TRI , I apologize for the delay, I was on vacation last week. I'll resume working on this today.

Reviewable status: 2 unresolved discussions, LGTM missing from assignee JoseBarreiros-TRI, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

bindings/pydrake/gym/_drake_gym_env.py line 257 at r4 (raw file):

Previously, JoseBarreiros-TRI wrote…

I'm not sure about the expected behavior here. I think that if the run gets to this point it means that the simulator faulted which would cause an unexpected behavior when called by the info_handler(). Consider setting the info to an empty dict or info["sim_runtime_error"]=True.

I don't want to add anything to info that isn't standard.
This is what motivated adding an info handler over explicitly adding info["timestamp"]

I think it's likely that simulator will still behave normally after most runtime errors, but this is a good point
I'd go for just returning an empty dict in this case, and adding documentation that says, e.g.,
"if the simulator emits a runtime error while advancing, the provided info handler will not be called and will return an empty dict instead".

Would that work?

…o add_timestamp_to_step_info

samzapo

Reviewable status: 2 unresolved discussions, LGTM missing from assignee JoseBarreiros-TRI, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @samzapo)

bindings/pydrake/examples/gym/envs/cart_pole.py line 418 at r4 (raw file):

Previously, JoseBarreiros-TRI wrote…

why not set this to None or better provide a nice example of an info handler

Done.

JoseBarreiros-TRI

Reviewable status: 1 unresolved discussion, LGTM missing from assignee JoseBarreiros-TRI, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @samzapo)

bindings/pydrake/gym/_drake_gym_env.py line 257 at r4 (raw file):

Previously, samzapo (Samuel Zapolsky) wrote…

I don't want to add anything to info that isn't standard.
This is what motivated adding an info handler over explicitly adding info["timestamp"]

I think it's likely that simulator will still behave normally after most runtime errors, but this is a good point
I'd go for just returning an empty dict in this case, and adding documentation that says, e.g.,
"if the simulator emits a runtime error while advancing, the provided info handler will not be called and will return an empty dict instead".

Would that work?

yes, that is reasonable.

samzapo

Reviewable status: 1 unresolved discussion, LGTM missing from assignee JoseBarreiros-TRI, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @samzapo)

bindings/pydrake/gym/_drake_gym_env.py line 257 at r4 (raw file):

Previously, JoseBarreiros-TRI wrote…

yes, that is reasonable.

Done.

JoseBarreiros-TRI

Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @samzapo)

jwnimmer-tri · 2024-11-01T23:56:17Z

+@sammy-tri for platform review per schedule, please.

sammy-tri

+(status: squashing now)

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees JoseBarreiros-TRI,sammy-tri(platform)

Merge remote-tracking branch 'upstream/master' into gcs-preprocessing-parallel [tools] Suppress more Valgrind false positives in MOSEK (RobotLocomotion#22140) Update parsing infrastructure (RobotLocomotion#22102) 1. MultibodyPlant::RegisterVisualGeometry() can now take a GeometryInstance. - Clean up documentation. - Extend tests to include what is *actually* done. 2. SDF parser makes use of new API. - Stop returning optional<unique_ptr<>> (simply return the unique ptr). - Instead of constructing, deconstructing, and reconstructing geometry instances, we just pass the instance. [schema] Add Rotation::Sample (RobotLocomotion#22113) This bring it on par with Transform::Sample. Move the warning on small-size PSD matrix to each solver backend. (RobotLocomotion#22136) [solvers] Fix Gurobi console logging config to happen first (RobotLocomotion#22134) [bindings] Re-enable some gym test cases on macOS (RobotLocomotion#22112) [drake_gym] Add info_handler callback (RobotLocomotion#21900) Set gradient sparsity pattern in LorentzConeConstraint and RotatedLorentzConeConstraint. (RobotLocomotion#22125) Merge master. Drop a TODO. Fix bug when inadvertantly trying to preprocess edges into the start and goal vertex. Hold a limited number of programs in memory at one time. Move the construction of the preprocessing program to a separate function for better portability. Merge up-to-date version of SolveInParallel. Merge in preprocessing solver options behavior from branch. wip lint Use the preprocessing solver options even if a specific preprocessing solver is not specified. Fix another segfault. Bring back the parallelization, revealing the segfault. Update this test to reflect the new error message. First pass at parallelizing PreprocessShortestPath. Fix a segfault when initial_guesses is unset. Temporarily disable the parallelism for debugging. finalize interface wip

added change for PR

6e4fa3b

jwnimmer-tri mentioned this pull request Sep 16, 2024

Add a "timestamp" item to the info return value from DrakeGymEnv.step(...) #21899

Closed

jwnimmer-tri assigned JoseBarreiros-TRI Sep 16, 2024

samzapo added 3 commits September 17, 2024 08:07

added info handler instead of explicitly providing timestamp

8fbdbe1

Merge remote-tracking branch 'upstream/master' into add_timestamp_to_…

ac1221b

…step_info

formatting

2db2023

jwnimmer-tri changed the title ~~Adds a "timestamp" item to the info return value from DrakeGymEnv.step(...)~~ [drake_gym] Add info_handler callback Sep 17, 2024

jwnimmer-tri added the release notes: feature This pull request contains a new feature label Sep 17, 2024

jwnimmer-tri reviewed Sep 17, 2024

View reviewed changes

samzapo added 2 commits September 18, 2024 09:26

simplified info_handler is None logic.

b473a1e

Merge remote-tracking branch 'upstream/master' into add_timestamp_to_…

66d6b4d

…step_info

JoseBarreiros-TRI reviewed Sep 18, 2024

View reviewed changes

samzapo commented Sep 18, 2024

View reviewed changes

(continued) simplified info_handler is None logic.

0e0c7c2

jwnimmer-tri reviewed Sep 18, 2024

View reviewed changes

linted

f9fcaaf

samzapo commented Sep 18, 2024

View reviewed changes

jwnimmer-tri reviewed Sep 18, 2024

View reviewed changes

JoseBarreiros-TRI reviewed Sep 23, 2024

View reviewed changes

JoseBarreiros-TRI reviewed Oct 1, 2024

View reviewed changes

samzapo commented Oct 1, 2024

View reviewed changes

samzapo added 2 commits October 1, 2024 07:45

Merge branch 'master' of https://github.com/RobotLocomotion/drake int…

6b9b2f8

…o add_timestamp_to_step_info

changes for PR

76f34c3

samzapo commented Oct 1, 2024

View reviewed changes

JoseBarreiros-TRI reviewed Oct 7, 2024

View reviewed changes

samzapo commented Oct 8, 2024

View reviewed changes

JoseBarreiros-TRI reviewed Oct 8, 2024

View reviewed changes

jwnimmer-tri assigned sammy-tri Nov 1, 2024

sammy-tri added the status: squashing now https://drake.mit.edu/reviewable.html#curated-commits label Nov 6, 2024

sammy-tri reviewed Nov 6, 2024

View reviewed changes

sammy-tri merged commit 87b95eb into RobotLocomotion:master Nov 6, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[drake_gym] Add info_handler callback #21900

[drake_gym] Add info_handler callback #21900

samzapo commented Sep 12, 2024 •

edited by jwnimmer-tri

Loading

jwnimmer-tri commented Sep 16, 2024 •

edited

Loading

JoseBarreiros-TRI commented Sep 16, 2024

samzapo commented Sep 16, 2024

jwnimmer-tri commented Sep 16, 2024

jwnimmer-tri left a comment

JoseBarreiros-TRI left a comment

samzapo left a comment

samzapo commented Sep 18, 2024

jwnimmer-tri left a comment

samzapo left a comment

jwnimmer-tri left a comment

JoseBarreiros-TRI left a comment

JoseBarreiros-TRI left a comment

samzapo left a comment

samzapo left a comment

JoseBarreiros-TRI left a comment

samzapo left a comment

JoseBarreiros-TRI left a comment

jwnimmer-tri commented Nov 1, 2024

sammy-tri left a comment

[drake_gym] Add info_handler callback #21900

[drake_gym] Add info_handler callback #21900

Conversation

samzapo commented Sep 12, 2024 • edited by jwnimmer-tri Loading

jwnimmer-tri commented Sep 16, 2024 • edited Loading

JoseBarreiros-TRI commented Sep 16, 2024

samzapo commented Sep 16, 2024

jwnimmer-tri commented Sep 16, 2024

jwnimmer-tri left a comment

Choose a reason for hiding this comment

JoseBarreiros-TRI left a comment

Choose a reason for hiding this comment

samzapo left a comment

Choose a reason for hiding this comment

samzapo commented Sep 18, 2024

jwnimmer-tri left a comment

Choose a reason for hiding this comment

samzapo left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

JoseBarreiros-TRI left a comment

Choose a reason for hiding this comment

JoseBarreiros-TRI left a comment

Choose a reason for hiding this comment

samzapo left a comment

Choose a reason for hiding this comment

samzapo left a comment

Choose a reason for hiding this comment

JoseBarreiros-TRI left a comment

Choose a reason for hiding this comment

samzapo left a comment

Choose a reason for hiding this comment

JoseBarreiros-TRI left a comment

Choose a reason for hiding this comment

jwnimmer-tri commented Nov 1, 2024

sammy-tri left a comment

Choose a reason for hiding this comment

samzapo commented Sep 12, 2024 •

edited by jwnimmer-tri

Loading

jwnimmer-tri commented Sep 16, 2024 •

edited

Loading