Skip to content
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

[RLlib] Require ApeX LR schedule test to produce learner info #27557

Merged
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions rllib/algorithms/apex_dqn/tests/test_apex_dqn.py
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,8 @@ def test_apex_lr_schedule(self):
)
.reporting(
min_sample_timesteps_per_iteration=10,
# Make sure that results contain info on default policy
min_train_timesteps_per_iteration=10,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In L141, we require results to contain info on default policy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can probably remove l126 then.

If we need to merge quickly though then not necessary to make the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! I've tested removing L126 now and it ends up messing with the way we test the LR here. So we should merge as is.

# 0 metrics reporting delay, this makes sure timestep,
# which lr depends on, is updated after each worker rollout.
min_time_s_per_iteration=0,
Expand Down