IsaacLab icon indicating copy to clipboard operation
IsaacLab copied to clipboard

Adds support for custom `ProgressReporter` to Ray integration

Open ozhanozen opened this issue 4 months ago • 3 comments

Description

This PR adds support for providing a custom ProgressReporter while doing hyperparameter tuning with Ray Integration.

Without the PR, the Ray integration defaults to the standard CLIReporter, which often displays metrics that aren’t particularly relevant or at the desired frequency. Similar to how we allow users to specify a --cfg_class (e.g., CartpoleTheiaJobCfg), this PR lets them optionally provide a custom ProgressReporter class. If such is not provided, it falls back to the default.

Moreover, I have added an example inside vision_cartpole_cfg.py (i.e., CustomCartpoleProgressReporter).

One point to highlight is that the new "context-aware progress reporting" conflicts with custom ProgressReporter, so if a custom ProgressReporter is provided, the PR disables the context-aware progress reporting.

Fixes #3268.

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist

  • [x] I have run the pre-commit checks with ./isaaclab.sh --format
  • [ ] I have made corresponding changes to the documentation
  • [x] My changes generate no new warnings
  • [ ] I have added tests that prove my fix is effective or that my feature works
  • [ ] I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • [x] I have added my name to the CONTRIBUTORS.md or my name already exists there

ozhanozen avatar Aug 25 '25 15:08 ozhanozen

@garylvov, would love to get your feedback if you have time!

ozhanozen avatar Aug 25 '25 15:08 ozhanozen

Hi thanks for the PR! (EDIT: didn't mean to mention the issue got this confused with early stopping)

garylvov avatar Aug 26 '25 01:08 garylvov

Greptile Overview

Greptile Summary

This PR adds support for custom ProgressReporter classes in Ray hyperparameter tuning, allowing users to customize metrics display and reporting frequency. The implementation:

  • Adds optional --progress_reporter CLI argument that accepts a class name from the config file
  • Disables Ray's context-aware progress reporting when custom reporter is used (via RAY_AIR_NEW_OUTPUT=0)
  • Validates that custom reporters don't set _metric or _mode attributes directly
  • Provides example implementation (CustomCartpoleProgressReporter) showing typical usage

Issues found:

  • Line 544: If user specifies a non-existent progress reporter class, code silently falls back to default instead of raising an error
  • Line 546-549: Doesn't handle case where user passes an already-instantiated reporter object (will incorrectly raise TypeError)

Confidence Score: 3/5

  • This PR has logical errors in error handling that will cause confusing behavior for users
  • Score reflects two critical logic bugs: (1) silently ignoring invalid reporter class names instead of raising errors, leading to confusing silent fallback behavior, and (2) incorrectly rejecting already-instantiated reporter objects. These will cause user-facing issues but don't affect core functionality when valid class names are provided.
  • Pay close attention to scripts/reinforcement_learning/ray/tuner.py lines 544-549 for the error handling logic

Important Files Changed

File Analysis

Filename Score Overview
scripts/reinforcement_learning/ray/tuner.py 3/5 Adds custom ProgressReporter support with validation logic, but has error handling gaps when class not found and doesn't handle instance inputs
scripts/reinforcement_learning/ray/hyperparameter_tuning/vision_cartpole_cfg.py 5/5 Adds example CustomCartpoleProgressReporter class demonstrating the feature - implementation looks correct

Sequence Diagram

sequenceDiagram
    participant User
    participant CLI as tuner.py CLI
    participant Module as Config Module
    participant Tuner as invoke_tuning_run
    participant Ray as Ray Tune

    User->>CLI: Run with --progress_reporter CustomReporter
    CLI->>Module: Load config file
    CLI->>Module: getattr(module, "CustomReporter")
    alt Class not found
        Module-->>CLI: hasattr returns False
        CLI->>CLI: progress_reporter = None (silent fallback)
    else Class found
        Module-->>CLI: Return CustomReporter class
        CLI->>CLI: Validate is subclass of ProgressReporter
        CLI->>CLI: Instantiate reporter
    end
    CLI->>Tuner: invoke_tuning_run(cfg, args, progress_reporter)
    alt Custom reporter provided
        Tuner->>Tuner: Set RAY_AIR_NEW_OUTPUT=0
        Tuner->>Tuner: Validate no _metric/_mode attributes
    end
    Tuner->>Ray: Create RunConfig with progress_reporter
    Ray->>Ray: Start tuning with custom reporter

greptile-apps[bot] avatar Nov 11 '25 14:11 greptile-apps[bot]

@kellyguo11 pinging on small diff PR that only touches Ray stuff that's been open for some while, can we please merge? Thank you!!!

garylvov avatar Nov 25 '25 01:11 garylvov