[BugFix] Fix flaky Pendulum spec check and collector preemption ordering test (#3798)#3856
[BugFix] Fix flaky Pendulum spec check and collector preemption ordering test (#3798)#3856discobot wants to merge 1 commit into
Conversation
…ing test (pytorch#3798) PendulumEnv._step let th drift past the Bounded(-pi, pi) observation/state spec, so check_env_specs failed for seeds whose random rollout crossed the boundary (8/500 seeds locally). Wrap the angle with angle_normalize like the pendulum tutorial already does; the dynamics only see th through sin and angle_normalize, both 2*pi-periodic, so trajectories are unchanged. test_multi_sync_data_collector_ordering asserted slow envs are preempted after exactly one step, but the stop flag is only raised once the fastest workers have reported and workers poll it once per step, so on loaded runners a slow env can legitimately log a second valid step (CI showed traj_ids [4, 4, -1, -1, -1]). Assert instead that preempted envs stop with a contiguous valid prefix shorter than a full rollout, check observations over the valid rows for all envs, and widen the slow-env step time to 0.3s for margin.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/rl/3856
Note: Links to docs will display an error until the docs builds have been completed.
|
|
Hi @discobot! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
meta-cla
Bot
added
the
CLA Signed
1 participant
Description
The three tracked flakes have two independent causes: a real bug in
PendulumEnv, and an over-constrained timing assertion in the collector ordering test.PendulumEnv lets
thdrift out of its spec._stepcomputesnew_th = th + new_thdot * dtwithout wrapping, while thethobservation/state spec isBounded(-pi, pi), socheck_env_specsfails whenever the random rollout pushes the angle past the boundary — 8/500 seeds locally. The failure is value-domain, not device-dependent, which is why both the cpu and cuda variants flake. Fixed by wrapping withangle_normalize, which the pendulum tutorial's copy of this env already does; behavior-preserving, since the dynamics seethonly throughsinandangle_normalize, both 2*pi-periodic. Added a deterministic regression test that drives the pendulum across +pi at max speed and asserts spec containment.The ordering test over-constrains preemption timing. It asserted slow envs are preempted after exactly one step, but the stop flag is only raised once the fastest workers have reported and workers poll it once per step, so on a loaded runner a slow env can legitimately log a second valid step before stopping (the CI failure shows traj_ids
[4, 4, -1, -1, -1]). The_Interruptoritself is sound; only the test needed recalibrating. It now asserts a preempted env stops with a contiguous valid prefix shorter than a full rollout and extends the observation==env_id check to the valid rows of preempted envs (previously skipped). Slow-env step time widened 0.1s -> 0.3s for margin.Tested: 500-seed
check_env_specssweep goes 8 failures -> 0; TestPendulum passes 100x repeat; the ordering test passes both normally and with an injected 0.25s stop-flag delay that reproduced the CI failure verbatim before the fix, plus 10x repeat under background CPU load.Motivation and Context
Fixes #3798.
Types of changes
Checklist