diff --git a/projects/rocprofiler-compute/CONTRIBUTING.md b/projects/rocprofiler-compute/CONTRIBUTING.md index 5c7bda24d6..a4bc406959 100644 --- a/projects/rocprofiler-compute/CONTRIBUTING.md +++ b/projects/rocprofiler-compute/CONTRIBUTING.md @@ -58,7 +58,7 @@ To ensure code quality and consistency, we use **Ruff**, a fast Python linter an ----- -## Installing and Running Ruff +### Installing and Running Ruff Ruff is available on PyPI and can be installed using `pip`: @@ -82,18 +82,17 @@ ruff format . ----- -```markdown -## Type Annotations +### Type Annotation Guidelines -This project enforces type annotations using Ruff's `flake8-annotations` rules. All new code must include proper type annotations. +This project enforces type annotations using Ruff's `flake8-annotations` rules (ANN). All new code in `src/` must include proper type annotations. -### Requirements +#### Requirements - All function arguments must have type annotations (except `self` and `cls`) - All function return types must be annotated - Class attributes should have type annotations where applicable -### Examples +#### Examples ```python # Good - properly annotated @@ -106,7 +105,7 @@ def process_kernel_data(kernel_name, metrics): return {"kernel": kernel_name, "avg": sum(metrics) / len(metrics)} ``` -### Checking Type Annotations +#### Checking Type Annotations To specifically check for missing type annotations: @@ -115,9 +114,66 @@ ruff check --select ANN . ``` For existing code, we're gradually adding type annotations. When modifying existing functions, please add type annotations to any code you touch. + +----- + +### String Formatting Guidelines + +This project enforces modern Python string formatting practices using Ruff's `pyupgrade` rules (UP). All new code in `src/` should use f-strings where applicable instead of older formatting methods. + +#### Requirements + +- Use f-strings for string formatting when variables or expressions need to be embedded +- Replace `.format()` method calls and `%` formatting with f-strings where possible +- F-strings are preferred for readability and performance + +#### Examples +```python +# Good - using f-strings +name = "kernel_analysis" +count = 42 +message = f"Processing {name} with {count} metrics" +path = f"{base_dir}/results/{filename}.csv" + +# Bad - will be caught by Ruff (UP045) +message = "Processing {} with {} metrics".format(name, count) +message = "Processing %s with %s metrics" % (name, count) +path = "{}/results/{}.csv".format(base_dir, filename) ``` -## Disabling Formatting for Specific Sections +----- + +### Path Handling Guidelines + +This project enforces modern Python path handling practices using Ruff's `flake8-use-pathlib` rules (PTH). All new code in `src/` should use `pathlib.Path` methods instead of legacy `os.path` functions for directory operations. + +#### Requirements + +- Use `pathlib.Path` methods for all path operations instead of `os.path` functions +- Use `Path.cwd()` instead of `os.getcwd()` +- Use `Path.exists()` instead of `os.path.exists()` +- Use `Path.is_file()` and `Path.is_dir()` instead of `os.path.isfile()` and `os.path.isdir()` +- Use the `/` operator for path joining instead of `os.path.join()` + +#### Examples +```python +# Good - using pathlib methods +current_dir = Path.cwd() +config_path = current_dir / "config" / "settings.yaml" +if config_path.exists() and config_path.is_file(): + # Process file + +# Bad - will be caught by Ruff (PTH rules) +import os +current_dir = Path(os.getcwd()) # PTH109 +config_path = os.path.join(current_dir, "config", "settings.yaml") # PTH118 +if os.path.exists(config_path) and os.path.isfile(config_path): # PTH110, PTH113 + # Process file +``` + +----- + +### Disabling Formatting for Specific Sections There may be instances where you need to disable Ruff's formatting on a specific block of code. You can do this using special comments: @@ -126,12 +182,12 @@ There may be instances where you need to disable Ruff's formatting on a specific You can also disable specific linting rules for a line by using `# noqa: `. -## Coding guidelines +### Coding guidelines Below are some repository specific guidelines which are followed througout the repository. Any future contributions should adhere to these guidelines: * Use the `pathlib` library functions instead of `os.path` for manipulating the file paths. -## Build and test documentation changes +### Build and test documentation changes -For instructions on how to build and test documentation changes (files under docs folder), please see https://rocm.docs.amd.com/en/latest/contribute/contributing.html +For instructions on how to build and test documentation changes (files under docs folder), please see https://rocm.docs.amd.com/en/latest/contribute/contributing.html \ No newline at end of file diff --git a/projects/rocprofiler-compute/pyproject.toml b/projects/rocprofiler-compute/pyproject.toml index 475dc24777..da826a2855 100644 --- a/projects/rocprofiler-compute/pyproject.toml +++ b/projects/rocprofiler-compute/pyproject.toml @@ -33,11 +33,15 @@ extend-exclude = [ [tool.ruff.lint] # Enable Pyflakes (F), pycodestyle (E, W for PEP8), aisort (I), # type annotation (ANN), and f-string (UP) rules. -select = ["E", "W", "F", "I", "ANN","UP"] -ignore = ["E713", "E711", "ANN001", "ANN401", "UP045"] +select = ["E", "W", "F", "I", "ANN", "UP", "PTH"] +ignore = ["E713", "E711", "UP045", "PTH123", "PTH207"] fixable = ["ALL"] unfixable = [] +[tool.ruff.lint.per-file-ignores] +# Ignore ANN and UP rules for all files except those in src/ +"!src/**" = ["ANN", "UP", "PTH"] + [tool.ruff.lint.flake8-annotations] allow-star-arg-any = true #allow Any for *args ignore-fully-untyped = false #require type annotations diff --git a/projects/rocprofiler-compute/src/argparser.py b/projects/rocprofiler-compute/src/argparser.py index 37274eacac..00e10b45b9 100644 --- a/projects/rocprofiler-compute/src/argparser.py +++ b/projects/rocprofiler-compute/src/argparser.py @@ -24,7 +24,6 @@ ############################################################################## import argparse -import os import re from pathlib import Path from typing import Optional @@ -153,10 +152,10 @@ Examples: metavar="", type=str, dest="path", - default=str(Path(os.getcwd()) / "workloads"), + default=str(Path.cwd() / "workloads"), required=False, help=( - f"\t\t\tSpecify path to save workload.\n\t\t\t(DEFAULT: {os.getcwd()}/workloads/)" # noqa: E501 + f"\t\t\tSpecify path to save workload.\n\t\t\t(DEFAULT: {Path.cwd()}/workloads/)" # noqa: E501 ), ) profile_group.add_argument( diff --git a/projects/rocprofiler-compute/src/rocprof_compute_base.py b/projects/rocprofiler-compute/src/rocprof_compute_base.py index e814c21c8e..31ea2219c6 100644 --- a/projects/rocprofiler-compute/src/rocprof_compute_base.py +++ b/projects/rocprofiler-compute/src/rocprof_compute_base.py @@ -25,7 +25,6 @@ import argparse import importlib -import os import socket import sys import time @@ -216,7 +215,7 @@ class RocProfCompute: def handle_profile_args(self) -> None: # Add --name to workload path if --path is not given - if self.__args.path == str(Path(os.getcwd()) / "workloads"): + if self.__args.path == str(Path.cwd() / "workloads"): if not hasattr(self.__args, "name") or not self.__args.name: console_error("-n/--name is required") self.__args.path = str(Path(self.__args.path) / self.__args.name) diff --git a/projects/rocprofiler-compute/src/rocprof_compute_profile/profiler_base.py b/projects/rocprofiler-compute/src/rocprof_compute_profile/profiler_base.py index 52be6cff39..07db98d93d 100644 --- a/projects/rocprofiler-compute/src/rocprof_compute_profile/profiler_base.py +++ b/projects/rocprofiler-compute/src/rocprof_compute_profile/profiler_base.py @@ -25,9 +25,6 @@ import argparse import csv -import glob -import os -import re import shlex import shutil import time @@ -129,7 +126,7 @@ class RocProfCompute_Base: # handle rocpd format if args.format_rocprof_output == "rocpd": # Vertically concat (by rows) results_*.csv into pmc_perf.csv - result_files = glob.glob(f"{args.path}/results_*.csv") + result_files = list(Path(args.path).glob("results_*.csv")) with open(output_file, "w", newline="") as outfile: writer = None @@ -148,7 +145,7 @@ class RocProfCompute_Base: # Delete results_*.csv files for file in result_files: - os.remove(file) + Path(file).unlink() console_debug(f"Deleted file: {file}") return @@ -158,27 +155,17 @@ class RocProfCompute_Base: files = [ file for pattern in csv_patterns - for file in glob.glob(f"{args.path}/{pattern}") + for file in Path(args.path).glob(pattern) ] if args.hip_trace: - # remove hip api trace ouputs from this list - files = [ - f - for f in files - if not re.compile(r"^.*_hip_api_trace\.csv$").match( - os.path.basename(f) - ) - ] + # remove hip api trace outputs from this list + files = [f for f in files if not f.name.endswith("_hip_api_trace.csv")] if args.kokkos_trace: - # remove marker api trace ouputs from this list + # remove marker api trace outputs from this list files = [ - f - for f in files - if not re.compile(r"^.*_marker_api_trace\.csv$").match( - os.path.basename(f) - ) + f for f in files if not f.name.endswith("_marker_api_trace.csv") ] elif isinstance(args.path, list): files = args.path @@ -344,7 +331,7 @@ class RocProfCompute_Base: for file in files: # Do not remove accumulate counter files if "SQ_" not in file or "SQC_" not in file: - os.remove(file) + Path(file).unlink() return None else: return df @@ -410,7 +397,7 @@ class RocProfCompute_Base: print_status(status_msg) # Run profiling on each input file - input_files = sorted(glob.glob(f"{args.path}/perfmon/*.txt")) + input_files = sorted(Path(args.path).glob("perfmon/*.txt")) total_runs = len(input_files) total_profiling_time = 0.0 @@ -440,7 +427,7 @@ class RocProfCompute_Base: "-i", "-r", f"s%^(kernel:).*%kernel: {','.join(self.__args.kernel)}%g", - fname, + str(fname), ]) # log output from profile filtering if not success: @@ -455,7 +442,7 @@ class RocProfCompute_Base: "-i", "-r", f"s%^(range:).*%range: {' '.join(self.__args.dispatch)}%g", - fname, + str(fname), ]) # log output from profile filtering if not success: @@ -471,10 +458,10 @@ class RocProfCompute_Base: "rocprofv3", "rocprofiler-sdk", ): - options = self.get_profiler_options(fname, self._soc) + options = self.get_profiler_options(str(fname), self._soc) start_time = time.time() run_prof( - fname=fname, + fname=str(fname), profiler_options=options, workload_dir=args.path, mspec=self._soc._mspec, @@ -500,8 +487,7 @@ class RocProfCompute_Base: ): return - input_files = glob.glob(f"{args.path}/perfmon/*.txt") - total_runs = len(input_files) + total_runs = len(list(Path(args.path).glob("perfmon/*.txt"))) console_log(f"[Run {total_runs + 1}/{total_runs + 1}][PC sampling profile run]") diff --git a/projects/rocprofiler-compute/src/rocprof_compute_soc/soc_base.py b/projects/rocprofiler-compute/src/rocprof_compute_soc/soc_base.py index bc87268996..0c0265502d 100644 --- a/projects/rocprofiler-compute/src/rocprof_compute_soc/soc_base.py +++ b/projects/rocprofiler-compute/src/rocprof_compute_soc/soc_base.py @@ -24,7 +24,6 @@ ############################################################################## import argparse -import glob import json import math import os @@ -293,8 +292,8 @@ class OmniSoC_Base: # File id dict config_root_dir = f"{args.config_dir}/{self.__arch}" config_filename_dict = { - Path(filename).name.split("_")[0]: filename - for filename in glob.glob(f"{config_root_dir}/*.yaml") + filename.name.split("_")[0]: str(filename) + for filename in Path(config_root_dir).glob("*.yaml") } filter_blocks = args.filter_blocks diff --git a/projects/rocprofiler-compute/src/rocprof_compute_tui/widgets/tabs/tabs_terminal.py b/projects/rocprofiler-compute/src/rocprof_compute_tui/widgets/tabs/tabs_terminal.py index 017f059654..4677c1922c 100644 --- a/projects/rocprofiler-compute/src/rocprof_compute_tui/widgets/tabs/tabs_terminal.py +++ b/projects/rocprofiler-compute/src/rocprof_compute_tui/widgets/tabs/tabs_terminal.py @@ -26,6 +26,7 @@ import os import platform import subprocess +from pathlib import Path from typing import Optional from rich.text import Text @@ -43,7 +44,7 @@ class Terimnal(Container): classes: Optional[str] = None, ) -> None: super().__init__(name=name, id=id, classes=classes) - self.current_directory = os.getcwd() + self.current_directory = Path.cwd() self.output_text: str = "" self.input_text: str = "" self.input_prompt: str = "" @@ -80,9 +81,7 @@ class Terimnal(Container): def update_prompt(self) -> None: """Update the command prompt in the input field.""" input_widget = self.query_one("#terminal-input") - current_path = ( - os.path.basename(self.current_directory) or self.current_directory - ) + current_path = Path(self.current_directory).name or self.current_directory if platform.system() != "Windows": prompt = f"{current_path} $ " @@ -156,15 +155,21 @@ class Terimnal(Container): path = command[3:].strip() if not path: # Just "cd" usually goes to home directory - path = os.path.expanduser("~") + new_path = Path.home() + else: + new_path = Path(path) - # Handle relative paths - if not os.path.isabs(path): - path = os.path.join(self.current_directory, path) + # Handle relative paths + if not new_path.is_absolute(): + new_path = Path(self.current_directory) / new_path + + # Resolve any symlinks and normalize the path + new_path = new_path.resolve() # Change to the new directory - os.chdir(path) - self.current_directory = os.getcwd() + new_path_str = str(new_path) + os.chdir(new_path_str) + self.current_directory = new_path_str self.add_output(f"Changed directory to {self.current_directory}\n") self.update_prompt() except Exception as e: diff --git a/projects/rocprofiler-compute/src/roofline.py b/projects/rocprofiler-compute/src/roofline.py index e7cd6a6074..5c39ade822 100644 --- a/projects/rocprofiler-compute/src/roofline.py +++ b/projects/rocprofiler-compute/src/roofline.py @@ -24,7 +24,6 @@ ############################################################################## import argparse -import os import textwrap import time from abc import abstractmethod @@ -164,7 +163,7 @@ class Roofline: base_path = Path(base_dir) - if base_path.name == "workloads" and base_path.parent == Path(os.getcwd()): + if base_path.name == "workloads" and base_path.parent == Path.cwd(): app_name = getattr(self.__args, "name", "default_app_name") gpu_model_name = getattr(self.__mspec, "gpu_model", "default_gpu_model") diff --git a/projects/rocprofiler-compute/src/utils/file_io.py b/projects/rocprofiler-compute/src/utils/file_io.py index adfe7e163f..d481bad39d 100644 --- a/projects/rocprofiler-compute/src/utils/file_io.py +++ b/projects/rocprofiler-compute/src/utils/file_io.py @@ -22,8 +22,6 @@ # THE SOFTWARE. ############################################################################## - -import os import re from collections import OrderedDict from pathlib import Path @@ -56,20 +54,18 @@ def load_panel_configs( """ configs: dict[int, dict[str, Any]] = {} for dir_path in dirs: - for root, _, files in os.walk(dir_path): - for file_name in files: - if file_name.endswith(".yaml"): - with open(Path(root) / file_name) as file: - config_yml = yaml.safe_load(file) - # metric key can be None due to some metric- - # tables not having any metrics - # metric key should be empty dict instead of None - panel_config = config_yml["Panel Config"] - for data_source in panel_config["data source"]: - metric_table = data_source.get("metric_table") - if metric_table and metric_table["metric"] is None: - metric_table["metric"] = {} - configs[panel_config["id"]] = panel_config + for yaml_file in Path(dir_path).rglob("*.yaml"): + with open(yaml_file) as file: + config_yml = yaml.safe_load(file) + # metric key can be None due to some metric- + # tables not having any metrics + # metric key should be empty dict instead of None + panel_config = config_yml["Panel Config"] + for data_source in panel_config["data source"]: + metric_table = data_source.get("metric_table") + if metric_table and metric_table["metric"] is None: + metric_table["metric"] = {} + configs[panel_config["id"]] = panel_config # TODO: sort metrics as the header order in case they- # are not defined in the same order @@ -207,33 +203,32 @@ def create_df_pmc( dfs: list[pd.DataFrame] = [] coll_levels: list[str] = [] - for root, _, files in os.walk(raw_data_dir): - for file_name in files: - # Process SQ*.csv or pmc_perf.csv files - is_sq_file = file_name.endswith(".csv") and file_name.startswith("SQ") - is_pmc_perf = file_name == f"{schema.PMC_PERF_FILE_PREFIX}.csv" + for csv_file in Path(raw_data_dir).rglob("*.csv"): + file_name = csv_file.name - if is_sq_file or is_pmc_perf: - file_path = Path(root) / file_name - tmp_df = pd.read_csv(file_path) + is_sq_file = file_name.startswith("SQ") + is_pmc_perf = file_name == f"{schema.PMC_PERF_FILE_PREFIX}.csv" - if config_dict.get("format_rocprof_output") == "rocpd": - tmp_df = rocpd_data.process_rocpd_csv(tmp_df) + if is_sq_file or is_pmc_perf: + tmp_df = pd.read_csv(csv_file) - # Demangle original KernelNames - kernel_name_shortener(tmp_df, kernel_verbose) + if config_dict.get("format_rocprof_output") == "rocpd": + tmp_df = rocpd_data.process_rocpd_csv(tmp_df) - # NB: - # Idealy, the Node column should be added out of - # multiindexing level. Here, we add it into pmc_perf - # as it is the main sub-df which can be handled easily - # later. - if file_name == "pmc_perf.csv" and node_name is not None: - tmp_df.insert(0, "Node", node_name) + # Demangle original KernelNames + kernel_name_shortener(tmp_df, kernel_verbose) - dfs.append(tmp_df) - # Remove .csv extension for collection level - coll_levels.append(file_name[:-4]) + # NB: + # Idealy, the Node column should be added out of + # multiindexing level. Here, we add it into pmc_perf + # as it is the main sub-df which can be handled easily + # later. + if file_name == "pmc_perf.csv" and node_name is not None: + tmp_df.insert(0, "Node", node_name) + + dfs.append(tmp_df) + # Remove .csv extension for collection level + coll_levels.append(csv_file.stem) if not dfs: return pd.DataFrame() diff --git a/projects/rocprofiler-compute/src/utils/mi_gpu_spec.py b/projects/rocprofiler-compute/src/utils/mi_gpu_spec.py index 1739228852..82a4e22beb 100644 --- a/projects/rocprofiler-compute/src/utils/mi_gpu_spec.py +++ b/projects/rocprofiler-compute/src/utils/mi_gpu_spec.py @@ -23,7 +23,7 @@ ############################################################################## -import os +from pathlib import Path from typing import Any, Optional import yaml @@ -103,8 +103,8 @@ class MIGPUSpecs: | -- memory partition mode """ - current_dir = os.path.dirname(__file__) - yaml_file_path = os.path.join(current_dir, "mi_gpu_spec.yaml") + current_dir = Path(__file__).parent + yaml_file_path = current_dir / "mi_gpu_spec.yaml" # Load the YAML data yaml_data = cls._load_yaml(yaml_file_path) diff --git a/projects/rocprofiler-compute/src/utils/utils.py b/projects/rocprofiler-compute/src/utils/utils.py index f990cb6759..4e963bf764 100644 --- a/projects/rocprofiler-compute/src/utils/utils.py +++ b/projects/rocprofiler-compute/src/utils/utils.py @@ -798,7 +798,7 @@ def run_prof( if retain_rocpd_output: shutil.copyfile( glob.glob(f"{workload_dir}/out/pmc_1/*/*.db")[0], - "f{workload_dir}/{fbase}.db", + f"{workload_dir}/{fbase}.db", ) console_warning( f"Retaining large raw rocpd database: {workload_dir}/{fbase}.db" @@ -853,6 +853,7 @@ def run_prof( f"Cannot write results for {fbase}.csv due to no counter " "csv files generated." ) + return # Combine results into single CSV file combined_results = pd.concat(