-
Notifications
You must be signed in to change notification settings - Fork 135
[rocprof-compute] Add Experimental Option #3143
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
base: develop
Are you sure you want to change the base?
Changes from all commits
082d5fe
17de161
7106e3d
5bdb24b
4333343
084b9df
73afbdb
40ec32f
11a753a
9c56064
7383ee2
b31de67
89545d4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
||
| 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`** | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The help kwargs should handle this, i.e. |
||
| 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 = [ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually you dont need registry with custom actions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| # --spatial-multiplexing | ||
xuchen-amd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| {"label": "Spatial multiplexing", "flags": ["--spatial-multiplexing"]}, | ||
| ] | ||
|
|
||
|
|
||
| def detect_experimental_config() -> bool: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This wont work, you will need to accept |
||
|
|
||
|
|
||
| def validate_block(value: str) -> str: | ||
| if METRIC_ID_RE.match(value): | ||
|
|
@@ -105,12 +200,26 @@ def add_general_group( | |
| "-s", "--specs", action="store_true", help="Print system specs and exit." | ||
| ) | ||
|
|
||
| general_group.add_argument( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
|
@@ -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" | ||
|
|
@@ -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") | ||
|
|
@@ -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, | ||
|
|
@@ -571,6 +676,24 @@ def omniarg_parser( | |
| # help="\t\t\tNumber of iterations (DEFAULT: 10)" | ||
| # ) | ||
|
|
||
| ## ---------------------------- | ||
| # Experimental Features | ||
| ## ---------------------------- | ||
|
|
||
| profile_group.add_argument( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implement custom action which handles warning/error/help all in one shot! And use the custom action as follows |
||
| "--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( | ||
|
|
@@ -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") | ||
|
|
@@ -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="", | ||
|
|
@@ -865,3 +983,20 @@ def omniarg_parser( | |
| "Enable it without node names means ALL." | ||
| ), | ||
| ) | ||
|
|
||
| ## ---------------------------- | ||
| # Experimental Features | ||
| ## ---------------------------- | ||
| analyze_group.add_argument( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use experimental action as mentioned above |
||
| "--spatial-multiplexing", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.", | ||
| ) | ||
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 needs to be updated to use custom actions instead of experimental registry