🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/rl/2042

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

• [PREEMPTIVE] - We recently implemented changes in pull job linux-jammy-py3.8-gcc11 / build

❌ 4 New Failures, 1 Unrelated Failure

As of commit 1987d4b with merge base a7bf5a4 ():

This comment was automatically generated by Dr. CI and updates every 15 minutes.

$\color{#D29922}\textsf{\Large⚠\kern{0.2cm}\normalsize Warning}$ Result of CPU Benchmark Tests

Total Benchmarks: 91. Improved: $\large\color{#35bf28}4$. Worsened: $\large\color{#d91a1a}4$.

$\color{#D29922}\textsf{\Large⚠\kern{0.2cm}\normalsize Warning}$ Result of GPU Benchmark Tests

Total Benchmarks: 94. Improved: $\large\color{#35bf28}4$. Worsened: $\large\color{#d91a1a}7$.

@vmoens I would raise a warning in collectors when set_truncated=True but reset_at_each_iter=False, or even raise an error.

@vmoens I would raise a warning in collectors when set_truncated=True but reset_at_each_iter=False, or even raise an error.

Can you elaborate? To me it's ok to have set_truncated=True when you don't reset. You will have trajectories that are slices of real ones (start of the batch isn't start of the episode) but at least you can delimitate what comes from where if you do a reshape(-1) or this sort of things, allowing you to feed that to GAE or other modules without worrying about one trajectory polluting another.

TLDR: my bad, I guess this is a valid thing to do, but if you have something that computes the episodic return out of that, it needs to be adjusted.

Okay, I see. I didn't have that use case in mind. I had the opposite assumptions:

1. When truncated happens in the middle of the rollout, it is set and a value estimator (GAE) will know to bootstrap and not sum over the next rewards. This is not an issue here.

2. When a rollout stops in the middle of a trajectory and you feed the trajectory to a GAE, it will bootstrap at the end because terminated is not set (truncated is not needed it already knows there are no more samples sum over)

2b. when you decide to rollout twice because for some reason your algorithm told you you need more data, you can concatenate the rollouts and the GAE will make use of more samples.

I thought you would never want to bootstrap (set truncated) when the next data coming can still be used to compute a value estimate (no reset) but I guess that's wrong. You can just want to compute your values on a fixed number of rollout steps.

I can think of another issue though. If you have something that computes the episodic return on the trajectories it will have the wrong signal.