Add option to disable gpu metrics collection - #65809
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a configuration option to disable GPU metrics collection in the Ray dashboard agent, which helps avoid unnecessary polling of GPU management libraries like NVML. The feedback highlights three key improvements: maintaining backward compatibility in DashboardAgent.__init__ by adjusting the parameter order, optimizing GpuMetricProvider to avoid instantiating GPU providers when metrics are disabled, and defensively accessing gpu_metrics_enabled using getattr to prevent potential AttributeError issues in tests or custom implementations.
| disable_metrics_collection: bool = False, | ||
| disable_gpu_metrics: bool = False, | ||
| is_head: bool = False, |
There was a problem hiding this comment.
Adding disable_gpu_metrics before is_head changes the positional argument order of DashboardAgent.__init__. If any callers or tests instantiate DashboardAgent using positional arguments (e.g., passing is_head positionally), this change will silently break backward compatibility by misaligning the arguments.
To maintain backward compatibility, please place disable_gpu_metrics after is_head (right before the * keyword-only argument marker).
| disable_metrics_collection: bool = False, | |
| disable_gpu_metrics: bool = False, | |
| is_head: bool = False, | |
| disable_metrics_collection: bool = False, | |
| is_head: bool = False, | |
| disable_gpu_metrics: bool = False, |
| def __init__(self, enable_metric_report: bool = True): | ||
| self._provider: Optional[GpuProvider] = None | ||
| self._enable_metric_report = True | ||
| self._enable_metric_report = enable_metric_report | ||
| self._providers = [NvidiaGpuProvider(), AmdGpuProvider()] |
There was a problem hiding this comment.
When enable_metric_report is False, there is no need to instantiate the GPU providers (NvidiaGpuProvider and AmdGpuProvider). Instantiating them unnecessarily consumes memory and could potentially trigger side effects (such as loading driver libraries or executing system queries) during initialization.
We can optimize this by only instantiating the providers when enable_metric_report is True.
| def __init__(self, enable_metric_report: bool = True): | |
| self._provider: Optional[GpuProvider] = None | |
| self._enable_metric_report = True | |
| self._enable_metric_report = enable_metric_report | |
| self._providers = [NvidiaGpuProvider(), AmdGpuProvider()] | |
| def __init__(self, enable_metric_report: bool = True): | |
| self._provider: Optional[GpuProvider] = None | |
| self._enable_metric_report = enable_metric_report | |
| self._providers = [NvidiaGpuProvider(), AmdGpuProvider()] if enable_metric_report else [] |
| self._gpu_metric_provider = GpuMetricProvider( | ||
| enable_metric_report=dashboard_agent.gpu_metrics_enabled | ||
| ) |
There was a problem hiding this comment.
Directly accessing dashboard_agent.gpu_metrics_enabled can raise an AttributeError if dashboard_agent is a mock object (common in unit tests) or a custom agent implementation that does not define this attribute.
Using getattr with a default value of True is a safer, more defensive approach that prevents potential runtime or test failures.
| self._gpu_metric_provider = GpuMetricProvider( | |
| enable_metric_report=dashboard_agent.gpu_metrics_enabled | |
| ) | |
| self._gpu_metric_provider = GpuMetricProvider( | |
| enable_metric_report=getattr(dashboard_agent, "gpu_metrics_enabled", True) | |
| ) |
Co-authored-by: Eslam Elnikety <eelnikety@microsoft.com> Signed-off-by: Eslam Elnikety <eelnikety@microsoft.com> Signed-off-by: Paweł Bęza <pawelbeza@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
9470ec7 to
9550497
Compare
Description
Ray’s dashboard reporter periodically polls NVML for GPU metrics. In some
environments, these calls can contend with other GPU operations and contribute
to kernel lockups or panics.
Reducing the polling frequency only mitigates the problem. Deployments using
external GPU observability need a way to disable NVML polling completely.
Related issues
Fixes #65808