-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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] Introduce Connector Metrics #31618
[RLlib] Introduce Connector Metrics #31618
Conversation
@@ -19,10 +21,17 @@ | |||
class ActionConnectorPipeline(ConnectorPipeline, ActionConnector): | |||
def __init__(self, ctx: ConnectorContext, connectors: List[Connector]): | |||
super().__init__(ctx, connectors) | |||
self.timers = defaultdict(_Timer) |
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.
I thought about putting this in the ConnectorPipeline class but it has no functionality related to timers if I do that and it would make the inheritance model which we have here even weirder.
Signed-off-by: Artur Niederfahrenhorst <[email protected]>
Signed-off-by: Artur Niederfahrenhorst <[email protected]>
Signed-off-by: Artur Niederfahrenhorst <[email protected]>
Signed-off-by: Artur Niederfahrenhorst <[email protected]>
fffe99b
to
8bef040
Compare
Signed-off-by: Artur Niederfahrenhorst <[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.
have a high level questions first.
thanks for the nice PR!
rllib/connectors/action/pipeline.py
Outdated
self.timers = defaultdict(_Timer) | ||
|
||
def reset(self, env_id: str): | ||
self.timers.clear() |
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.
maybe we don't want to reset timers.
like nobody is resetting action connector right now.
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.
Changed, thanks!
rllib/evaluation/env_runner_v2.py
Outdated
# Create connector metrics | ||
connector_metrics = {} | ||
for policy_id, policy in self._worker.policy_map.items(): | ||
connector_metrics[policy_id] = policy.get_connector_throughput_metrics() |
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.
this could be pretty costly. any chance we can only update the policies that are used for these episodes?
at the very least, let's only update the cached poclies.
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.
Changed, thanks! 👍
Signed-off-by: Artur Niederfahrenhorst <[email protected]>
Signed-off-by: Artur Niederfahrenhorst <[email protected]>
Signed-off-by: Artur Niederfahrenhorst <[email protected]>
a lot of test failure. can you take a look / re-trigger? |
Signed-off-by: Artur Niederfahrenhorst <[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.
looks awesome now. thanks. gonna merge.
Signed-off-by: Artur Niederfahrenhorst <[email protected]> Signed-off-by: Andrea Pisoni <[email protected]>
Signed-off-by: Artur Niederfahrenhorst <[email protected]>
Signed-off-by: Artur Niederfahrenhorst [email protected]
Why are these changes needed?
In order to properly better understand the influence of connector runtime, this PR introduces a first set of metrics and makes the necessary changes to support easy addition of more metrics related to connectors.
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.