Skip to content
Draft
3 changes: 3 additions & 0 deletions projects/rocprofiler-compute/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ Full documentation for ROCm Compute Profiler is available at [https://rocm.docs.

* Detection of MPI ranks while profiling and creation of output directories based on MPI rank.

* Added `--experimental` flag to enable experimental features that are under development. This flag is required when using any experimental features.
* Use `rocprof-compute --experimental --help` to see currently available experimental features.

### Changed

* Default output format for the underlying ROCprofiler-SDK tool has been changed from ``csv`` to ``rocpd``.
Expand Down
11 changes: 11 additions & 0 deletions projects/rocprofiler-compute/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -503,6 +503,17 @@ add_test(
tests/test_utils.py ${WORKING_DIR_OPTION}
)

# -----------------------------------
# Experimental flag/Misc tests
# -----------------------------------

add_test(
NAME test_misc
COMMAND
${PYTHON_TEST_COMMAND} -m pytest --junitxml=tests/test_misc.xml ${COV_OPTION}
tests/test_misc.py ${WORKING_DIR_OPTION}
)

# -----------------------------------
# Autogenerated configuration tests
# -----------------------------------
Expand Down
81 changes: 81 additions & 0 deletions projects/rocprofiler-compute/CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,87 @@ and apply your changes there. For more help reference GitHub's ['About Forking']
> [!TIP]
> To ensure you meet all formatting requirements before publishing, we recommend you utilize our included [*pre-commit hooks*](https://pre-commit.com/#introduction). For more information on how to use pre-commit hooks please see the [section below](#using-pre-commit-hooks).


### Adding Experimental Features
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be updated to use custom actions instead of experimental registry


This project uses a centralized registry for experimental features in `src/argparser.py`. The experimental flag system allows users to opt-in to unstable or preview features that are under development.

#### How It Works

The `--experimental` flag acts as a master toggle that:
- Shows help text for experimental features when enabled (prefixed with "EXPERIMENTAL:")
- Hides experimental options completely when disabled (using `argparse.SUPPRESS`)
- Prevents usage of experimental features without the flag (raises parser error)
- Displays a warning when experimental features are used
- Allows gradual rollout of new features without affecting stable functionality

#### Adding a New Experimental Feature

To add a new experimental feature, follow these 2 steps:

**1. Register the feature in `EXPERIMENTAL_FEATURES`**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think we need this registry, developers would have to add ExperimentalAction to their options anyways, you might as well move this information into the code where we print help message


Add an entry to the `EXPERIMENTAL_FEATURES` list in `src/argparser.py`:

```python
EXPERIMENTAL_FEATURES = [
# --spatial-multiplexing
{"label": "Spatial multiplexing", "flags": ["--spatial-multiplexing"]},
# Your new feature
{"label": "Your feature description", "flags": ["--your-flag"]},
]
```

**2. Add the option to the appropriate parser mode using `ExperimentalAction`**

Add your argument to the relevant parser (profile, analyze, etc.) using the `ExperimentalAction` custom action:

```python
profile_group.add_argument(
"--your-flag",
dest="your_flag",
required=False,
default=None,
action=ExperimentalAction,
experimental_enabled=experimental_enabled,
feature_label="Your feature description",
help_indent="\t\t\t", # "\t\t" if analyze_group
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The help kwargs should handle this, i.e. help=\t\tHELP MESSAGE, the code which injects EXPERIMENTAL: should take care of preserving the leading whitespaces, this way we can more easily promote features.

type=str, # Optional: specify type if needed
nargs="*", # Optional: specify nargs if needed
metavar="", # Optional: specify metavar for help text
help="Description of your feature",
)
```

The `ExperimentalAction` class automatically:
- Suppresses help text when `experimental_enabled=False`
- Prefixes help text with "EXPERIMENTAL:" when enabled
- Raises an error if the feature is used without `--experimental`
- Displays a warning message when the feature is used


#### Promoting Features to Stable

When a feature is ready to graduate from experimental to stable:

1. Remove the entry from the `EXPERIMENTAL_FEATURES` list
2. Change `action=ExperimentalAction` to `action="store_true"` (or appropriate action)
3. Remove the `experimental_enabled`, `feature_label`, and `help_indent` parameters
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

help_indent is redundant, help should take care of this and injection logic should preserve leading whitespaces

4. Update the help text to remove "EXPERIMENTAL:" prefix
5. Update any relevant documentation

#### Testing Experimental Features

Users can enable experimental features by passing the `--experimental` flag:

```bash
# View available experimental features (in profile mode)
rocprof-compute profile --experimental --help
```

Without `--experimental`, experimental features remain hidden and will raise an error if used.


## Using pre-commit hooks

Our project supports optional [*pre-commit hooks*](https://pre-commit.com/#introduction) which developers can leverage to verify formatting before publishing their code. Once enabled, any commits you propose to the repository will be automatically checked for formatting. Initial setup is as follows:
Expand Down
177 changes: 156 additions & 21 deletions projects/rocprofiler-compute/src/argparser.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,103 @@
from pathlib import Path
from typing import Optional

from utils.logger import console_warning
from utils.utils import METRIC_ID_RE

# Experimental Feature Registry
EXPERIMENTAL_FEATURES = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually you dont need registry with custom actions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that logically the registry is not needed. But kept for ease of management purposes (developers can see all the features in one place and helps to have a cleaner help menu message)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can hardcode the label and flags for experimental features in the help message of --experimental, also need to add this to contributing guide.
Having this as a global variable does not make much sense if only --experimental option help message is going to use this

# --spatial-multiplexing
{"label": "Spatial multiplexing", "flags": ["--spatial-multiplexing"]},
]


def detect_experimental_config() -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the code inside this function into rocprof_compute_base.py under parse_args(), this does not need to be a function if nothing is going to re-use it

"""
Detect if --experimental flag is present (for help text control).
"""
# Preliminary parser for --experimental
prelim_parser = argparse.ArgumentParser(add_help=False)
prelim_parser.add_argument("--experimental", action="store_true", default=False)

# Parse only known args (respects -- separator)
prelim_args, _ = prelim_parser.parse_known_args()

return prelim_args.experimental


class ExperimentalAction(argparse.Action):
"""
Custom action that enforces experimental feature gating.
- Suppresses help text when experimental mode is disabled
- Errors if feature used without --experimental flag
- Warns when experimental feature is used
"""

def __init__(
self,
option_strings: list[str],
dest: str,
experimental_enabled: bool = False,
feature_label: str = "",
help_indent: str = "\t\t\t",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per my other comments help_indent is redundant

nargs=None, # noqa: ANN001
const=None, # noqa: ANN001
default=None, # noqa: ANN001
type=None, # noqa: ANN001
choices=None, # noqa: ANN001
required: bool = False,
metavar=None, # noqa: ANN001
help: Optional[str] = None,
Comment on lines +70 to +77
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think apart from help everything else will be covered by **kwargs, so lets remove the None stuff

**kwargs, # noqa: ANN003
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think ruff should have a problem with this, is there a way to avoid these # noqa tags

) -> None:
self.experimental_enabled = experimental_enabled
self.feature_label = feature_label

# Modify help text based on experimental mode
if experimental_enabled:
help = (
f"{help_indent}EXPERIMENTAL: {help}"
if help
else f"{help_indent}EXPERIMENTAL"
)
else:
help = argparse.SUPPRESS

# Initialize with the help text and standard argparse.Action behavior
super().__init__(
option_strings=option_strings,
dest=dest,
nargs=nargs,
const=const,
default=default,
type=type,
choices=choices,
required=required,
help=help,
metavar=metavar,
**kwargs, # noqa: ANN001
)

def __call__(
self,
parser: argparse.ArgumentParser,
namespace: argparse.Namespace,
values, # noqa: ANN001,
option_string: Optional[str] = None, # noqa: ANN001, ARG002
) -> None:
# Error if experimental feature used without --experimental flag
if not self.experimental_enabled:
parser.error(
f"{self.feature_label} is an experimental feature. "
f"Use --experimental to enable it."
)

console_warning(
f"{self.feature_label} is experimental and may change in future releases."
)

setattr(namespace, self.dest, values)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wont work, you will need to accept base_action in the constructor and set inner_action attribute in this class (can be store_true, store and other variants).
Then, in __call__ you invoke this inner_action, this is required because options can have complex base/inner actions to handle.
Simply self.dest = value wont work for all options since they can have actions like store_true, store_false or storing list of arguments for a option and so on...
I believe one of my previous comments mentioned how to use inner_action



def validate_block(value: str) -> str:
if METRIC_ID_RE.match(value):
Expand Down Expand Up @@ -105,12 +200,26 @@ def add_general_group(
"-s", "--specs", action="store_true", help="Print system specs and exit."
)

general_group.add_argument(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The entire experimental registry should be shown to users regardless of whether --experimental flag was used, its okay to hide the experimental option but it should still be listed under this help message

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With custom actions, you can hardcode the list of experimental features here

"--experimental",
action="store_true",
default=False,
help=(
"Enable experimental feature(s):\n"
+ "".join(
f" {f['label']} ({' '.join(f['flags'])})\n"
for f in EXPERIMENTAL_FEATURES
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can hardcode the registry here, and mention in the contributing guide to adjust this help message for changes in experimental features

)
),
)


def omniarg_parser(
parser: argparse.ArgumentParser,
rocprof_compute_home: Path,
supported_archs: dict[str, str],
rocprof_compute_version: dict[str, Optional[str]],
experimental_enabled: bool = False,
) -> None:
# -----------------------------------------
# Parse arguments (dependent on mode)
Expand All @@ -119,7 +228,10 @@ def omniarg_parser(
## General Command Line Options
## ----------------------------
add_general_group(
parser, rocprof_compute_home, supported_archs, rocprof_compute_version
parser,
rocprof_compute_home,
supported_archs,
rocprof_compute_version,
)
parser._positionals.title = "Modes"
parser._optionals.title = "Help"
Expand Down Expand Up @@ -155,7 +267,10 @@ def omniarg_parser(
profile_parser._optionals.title = "Help"

add_general_group(
profile_parser, rocprof_compute_home, supported_archs, rocprof_compute_version
profile_parser,
rocprof_compute_home,
supported_archs,
rocprof_compute_version,
)
profile_group = profile_parser.add_argument_group("Profile Options")
roofline_group = profile_parser.add_argument_group("Standalone Roofline Options")
Expand Down Expand Up @@ -387,16 +502,6 @@ def omniarg_parser(
nargs=argparse.REMAINDER,
help="\t\t\tProvide command for profiling after double dash.",
)
profile_group.add_argument(
"--spatial-multiplexing",
type=int,
metavar="",
nargs="+",
dest="spatial_multiplexing",
required=False,
default=None,
help="\t\t\tProvide Node ID and GPU number per node.",
)
profile_group.add_argument(
"--format-rocprof-output",
required=False,
Expand Down Expand Up @@ -571,6 +676,24 @@ def omniarg_parser(
# help="\t\t\tNumber of iterations (DEFAULT: 10)"
# )

## ----------------------------
# Experimental Features
## ----------------------------

profile_group.add_argument(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use experiemntal action to show warnings and errors when this option is used with and without --experimental respectively.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implement custom action which handles warning/error/help all in one shot!

class ExperimentalAction(argparse.Action):                                                                                                                                                                                                               
      """                                                                                                                                                                                                                                                  
      Adds experimental validation to arguments.                                                                                                                                                                                                           
      Errors without --experimental, warns when used, modifies help text.
      """

      def __init__(
          self,
          option_strings: list[str],
          dest: str,
          experimental_enabled: bool = False,
          feature_label: str = "",
          base_action: str = 'store',
          help: Optional[str] = None,
          **kwargs
      ) -> None:
          self.experimental_enabled = experimental_enabled
          self.feature_label = feature_label

          # Modify help text based on experimental mode
          if experimental_enabled:
              help = f"EXPERIMENTAL: {help}" if help else "EXPERIMENTAL"
          else:
              help = argparse.SUPPRESS

          # Map action name to argparse action class
          # Add more action types here as needed (e.g., 'append', 'count', 'store_const')
          action_map = {
              'store': argparse._StoreAction,
              'store_true': argparse._StoreTrueAction,
          }
          action_class = action_map.get(base_action, argparse._StoreAction)

          # Create the inner action instance
          self.inner_action = action_class(option_strings, dest, help=help, **kwargs)
          super().__init__(option_strings, dest, help=help, **kwargs)

      def __call__(
          self,
          parser: argparse.ArgumentParser,
          namespace: argparse.Namespace,
          values: Any,
          option_string: Optional[str] = None
      ) -> None:
          # Import here to avoid circular dependency with utils.logger
          from utils.logger import console_warning

          # Error if experimental feature used without --experimental flag
          if not self.experimental_enabled:
              parser.error(
                  f"{self.feature_label} is an experimental feature. "
                  f"Use --experimental to enable it."
              )

          # Warn user that this is experimental
          console_warning(
              f"{self.feature_label} is experimental and may change in future releases."
          )

          # Delegate to the inner action
          self.inner_action(parser, namespace, values, option_string)

And use the custom action as follows

#  BEFORE:
  analyze_group.add_argument(
      "--spatial-multiplexing",
      dest="spatial_multiplexing",
      required=False,
      default=False,
      action="store_true",                                         # ← REMOVE this line
      help=experimental_help(experimental_enabled, "Mode of spatial multiplexing."),  # ← REMOVE wrapper
  )

#  AFTER:
  analyze_group.add_argument(
      "--spatial-multiplexing",
      dest="spatial_multiplexing",
      required=False,
      default=False,
      action=ExperimentalAction,                                   # ← CHANGE from "store_true"
      base_action='store_true',                                    # ← ADD (specify inner action)
      experimental_enabled=experimental_enabled,                   # ← ADD
      feature_label="Spatial multiplexing",                        # ← ADD
      help="Mode of spatial multiplexing.",                        # ← CHANGE to plain text (no wrapper)
  )

"--spatial-multiplexing",
dest="spatial_multiplexing",
required=False,
default=None,
action=ExperimentalAction,
experimental_enabled=experimental_enabled,
feature_label="Spatial multiplexing",
type=int,
nargs="*",
metavar="",
help="Provide Node ID and GPU number per node.",
)

## Analyze Command Line Options
## ----------------------------
analyze_parser = subparsers.add_parser(
Expand All @@ -595,7 +718,10 @@ def omniarg_parser(
analyze_parser._optionals.title = "Help"

add_general_group(
analyze_parser, rocprof_compute_home, supported_archs, rocprof_compute_version
analyze_parser,
rocprof_compute_home,
supported_archs,
rocprof_compute_version,
)
analyze_group = analyze_parser.add_argument_group("Analyze Options")
analyze_advanced_group = analyze_parser.add_argument_group("Advanced Options")
Expand Down Expand Up @@ -656,14 +782,6 @@ def omniarg_parser(
nargs="+",
help="\t\tSpecify GPU id(s) for filtering.",
)
analyze_group.add_argument(
"--spatial-multiplexing",
dest="spatial_multiplexing",
required=False,
default=False,
action="store_true",
help="\t\tMode of spatial multiplexing.",
)
analyze_group.add_argument(
"--output-format",
metavar="",
Expand Down Expand Up @@ -865,3 +983,20 @@ def omniarg_parser(
"Enable it without node names means ALL."
),
)

## ----------------------------
# Experimental Features
## ----------------------------
analyze_group.add_argument(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use experimental action as mentioned above

"--spatial-multiplexing",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You would have to provide a base_action

dest="spatial_multiplexing",
required=False,
default=False,
action=ExperimentalAction,
experimental_enabled=experimental_enabled,
feature_label="Spatial multiplexing",
help_indent="\t\t",
nargs=0,
const=True,
help="Mode of spatial multiplexing.",
)
12 changes: 11 additions & 1 deletion projects/rocprofiler-compute/src/rocprof_compute_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,12 @@ def load_soc_specs(self, sysinfo: Optional[dict] = None) -> None:
self.__soc[arch] = soc_class(self.__args, self.__mspec)

def parse_args(self) -> None:
from argparser import detect_experimental_config

# Detect if --experimental flag is present (for help text control)
experimental_requested: bool = detect_experimental_config()

# Build full parser with experimental knowledge
parser = argparse.ArgumentParser(
description=(
"Command line interface for AMD's GPU profiler, ROCm Compute Profiler"
Expand All @@ -286,7 +292,11 @@ def parse_args(self) -> None:
usage="rocprof-compute [mode] [options]",
)
omniarg_parser(
parser, config.rocprof_compute_home, self.__supported_archs, self.__version
parser,
config.rocprof_compute_home,
self.__supported_archs,
self.__version,
experimental_requested,
)
self.__args = parser.parse_args()

Expand Down
Loading
Loading