-
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] Aggregate impala learner info #25856
[RLlib] Aggregate impala learner info #25856
Conversation
@@ -795,7 +795,8 @@ def place_processed_samples_on_learner_queue(self) -> None: | |||
|
|||
def process_trained_results(self) -> ResultDict: | |||
# Get learner outputs/stats from output queue. |
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.
The comment is outdated here. Could you explain via some more one-line comments what we do here?
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.
ofc
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.
Looks great! thanks for the fix @avnishn . Could you just add a few more one-line comments explaining why we sometimes need to deepcopy the threads infos (when there is nothing to process ...)?
@@ -795,7 +795,8 @@ def place_processed_samples_on_learner_queue(self) -> None: | |||
|
|||
def process_trained_results(self) -> ResultDict: | |||
# Get learner outputs/stats from output queue. |
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.
# Get learner outputs/stats from output queue. | |
# Combine learner stats from the learner queue and update relevant timestep counters |
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.
what do you think of this @sven1977
Aggregate learner infos for impala training step fn
Why are these changes needed?
Related issue number
Checks
scripts/format.sh
to lint the changes in this PR.