-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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] Move DQN into the TargetNetworkAPI (and deprecate RLModuleWithTargetNetworksInterface
).
#46752
[RLlib] Move DQN into the TargetNetworkAPI (and deprecate RLModuleWithTargetNetworksInterface
).
#46752
Conversation
Signed-off-by: sven1977 <[email protected]>
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.
LGTM. Can we test, if this still works out with multi-learner setups?
@@ -105,7 +106,7 @@ def __init__(self, algo_class=None): | |||
self.num_multi_gpu_tower_stacks = 1 | |||
self.minibatch_buffer_size = 1 | |||
self.num_sgd_iter = 1 | |||
self.target_update_frequency = 1 | |||
self.target_network_update_freq = 1 |
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.
Way better!
lambda mid, module: ( | ||
module.sync_target_networks(tau=1.0) | ||
if hasattr(module, "sync_target_networks") | ||
lambda mid, mod: ( |
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.
Nice!
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.
And .... gone :)
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.
Yeah, this was such a hack.
Good point! WE haven't re-merged the +GPU +multi-GPU +multi-agent tests for DQN + SAC yet, correct? |
Signed-off-by: sven1977 <[email protected]>
No, we haven't because they are flaky on the CI machines. |
Move DQN into the TargetNetworkAPI (and deprecate
RLModuleWithTargetNetworksInterface
).TorchDDPRLModuleWithTargetNetworksInterface
.Why are these changes needed?
Related issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.