Add ExtendedTestBase shared base class for benchmark and functional tests by lajagapp · Pull Request #3689 · ROCm/TheRock

lajagapp · 2026-02-28T11:41:15Z

Summary

Extracts shared test infrastructure from BenchmarkBase into a new ExtendedTestBase base class, reducing code duplication between benchmark and functional tests.
This addresses the reviewer feedback on #3648 about sharing common logic between benchmark_base.py and functional_base.py.

Changes

New: `utils/extended_test_base.py`

Created ExtendedTestBase class with following shared infrastructure:
execute_command(), get_gpu_architecture(), detect_gpu_count(), create_test_result(), calculate_statistics(), upload_results()

Updated: `benchmark/scripts/benchmark_base.py`

BenchmarkBase now inherits from ExtendedTestBase
Removed duplicated methods: execute_command, _detect_gpu_count, calculate_statistics, upload_results

Updated: `benchmark/scripts/test_rccl_benchmark.py`

self._detect_gpu_count() → self.detect_gpu_count() (now inherited from base)

Updated: `utils/init.py`, `README.md`, `utils/README.md`

Added ExtendedTestBase to exports and documentation

Inheritance Hierarchy

ExtendedTestBase (utils/extended_test_base.py)
├── BenchmarkBase (benchmark/scripts/benchmark_base.py)
│   ├── ROCfftBenchmark, RCCLBenchmark, ROCblasBenchmark, ...
└── FunctionalBase (functional/scripts/functional_base.py)  ← will inherit in follow-up PR
    ├── MIOpenDriverConv, RcclTestInfra, ...

Follow-up

Update FunctionalBase to inherit from ExtendedTestBase (after this PR merges)

…ests Signed-off-by: Lenine Ajagappane <Lenine.Ajagappane@amd.com>

Signed-off-by: Lenine Ajagappane <Lenine.Ajagappane@amd.com>

geomin12

letting other reviewers review

geomin12

letting other folks review - if this isn't reviewed till fri, i will take a look

geomin12

lgtm, but can we post some logs making sure this works?

lajagapp · 2026-03-05T10:02:04Z

lgtm, but can we post some logs making sure this works?

Triggerd nighty-ci benchmark runs with this change - https://github.com/ROCm/TheRock/actions/runs/22701813100/job/65820378187

geomin12

letting other folks review - if this isn't reviewed till fri, i will take a look

geomin12 · 2026-03-09T15:52:35Z

+    """Base class providing shared infrastructure for benchmark and functional tests."""
+
+    def __init__(self, test_name: str, display_name: str = None):
+        """Initialize common test infrastructure.


Suggested change

"""Initialize common test infrastructure.

"""Initialize common extended test infrastructure.

Updated docstring as suggested.

geomin12 · 2026-03-09T15:52:43Z

+
+
+class ExtendedTestBase:
+    """Base class providing shared infrastructure for benchmark and functional tests."""


Suggested change

"""Base class providing shared infrastructure for benchmark and functional tests."""

"""Base class providing shared infrastructure for extended tests."""

Updated docstring.

geomin12 · 2026-03-09T15:53:43Z

+        process.wait()
+        return process.returncode
+
+    def get_gpu_architecture(self) -> str:


is this needed, seems identical to get_first_gpu_architecture

No, I just added get_gpu_architecture as a wrapper on top of get_first_gpu_architecture which is not required. So, removed it now.

geomin12 · 2026-03-09T15:53:59Z

+                "Ensure ROCm drivers are installed and GPU is accessible."
+            ) from e
+
+    def detect_gpu_count(self) -> int:


identical to get_visible_gpu_count, can be removed as it is part of the shared utils lib

No, I just added detect_gpu_count as a wrapper on top of get_visible_gpu_count with logging which is not required. So, removed it and updated the benchmark script to directly call get_visible_gpu_count.

geomin12 · 2026-03-09T15:54:13Z

+    ) -> Dict[str, Any]:
+        """Create a standardized test result dictionary.
+
+        Builds the base result structure used by both benchmark and functional tests.


Suggested change

Builds the base result structure used by both benchmark and functional tests.

Builds the base result structure used by extended tests.

Updated docstring as suggested.

geomin12 · 2026-03-09T15:55:04Z

+            if key == "pass":
+                key = "passed"
+            elif key == "fail":
+                key = "failed"


can we just update the keys to be passed or failed? not sure why we do this conversion

Yes removed now, this conversion logic not really needed here.

geomin12 · 2026-03-09T15:55:52Z

+            metadata.update(extra_metadata)
+
+        log.info(f"Uploading {test_type.title()} Test Results to API")
+        success = self.client.upload_results(


i believe the benchmark tests are passing but we still get the 403 error. can we remove this or comment this out until it is fixed?

open a GH issue with error logs and comment this out with a TODO?

Yes, this needs changes in LKG comparison and result handling. Can I create separate PR for this, so that we can get this PR reviewed and merged quickly which blocking other reviews.

I've created github-issue for #3850. Will create new PR and add for review.

Created new PR to temporarily disable the API upload - #3853.

geomin12 · 2026-03-09T15:56:36Z

+        if success:
+            log.info("Results uploaded successfully")
+        else:
+            log.info("Results saved locally only (API upload disabled or failed)")


can we add error log? this is a pretty vague message and if i got an error, i would want to see more than this

Just updated to print the generic error message now. Will take care of detailed errors as part of new PR which disables API upload.

geomin12 · 2026-03-09T15:56:46Z

 ## Usage

-### From Benchmark Scripts
+### From Test Base Classes


Suggested change

### From Test Base Classes

### From Extended Test Base Classes

- Remove get_gpu_architecture & detect_gpu_count and use shared utils lib - Update docstrings Signed-off-by: Lenine Ajagappane <Lenine.Ajagappane@amd.com>

…OCm/TheRock into users/lajagapp/add-test-helper

Signed-off-by: Lenine Ajagappane <Lenine.Ajagappane@amd.com>

geomin12

lot cleaner, thanks for updates

Add ExtendedTestBase shared base class for benchmark and functional t…

9e4d12f

…ests Signed-off-by: Lenine Ajagappane <Lenine.Ajagappane@amd.com>

github-project-automation Bot added this to TheRock Triage Feb 28, 2026

github-project-automation Bot moved this to TODO in TheRock Triage Feb 28, 2026

lajagapp added 2 commits February 28, 2026 11:42

Fix pre-commit errors

336d9c7

Signed-off-by: Lenine Ajagappane <Lenine.Ajagappane@amd.com>

Update extended_test_base.py

8fdf6a7

Signed-off-by: Lenine Ajagappane <Lenine.Ajagappane@amd.com>

lajagapp requested review from amd-shiraz, geomin12, jayhawk-commits and kiranzangam March 1, 2026 16:20

Merge branch 'main' into users/lajagapp/add-test-helper

ac7a9cf

lajagapp marked this pull request as ready for review March 1, 2026 16:20

lajagapp mentioned this pull request Mar 1, 2026

extended_tests: Add functional test base class and MIOpen convolution test #3648

Merged

geomin12 reviewed Mar 2, 2026

View reviewed changes

lajagapp requested a review from geomin12 March 4, 2026 18:06

geomin12 reviewed Mar 5, 2026

View reviewed changes

lajagapp requested a review from geomin12 March 5, 2026 10:03

geomin12 reviewed Mar 5, 2026

View reviewed changes

lajagapp requested a review from geomin12 March 7, 2026 02:54

geomin12 reviewed Mar 9, 2026

View reviewed changes

geomin12 mentioned this pull request Mar 9, 2026

Add new HeCBench SPIR-V test harness #3840

Closed

lajagapp added 3 commits March 9, 2026 17:20

Fix review comments

b22f3db

- Remove get_gpu_architecture & detect_gpu_count and use shared utils lib - Update docstrings Signed-off-by: Lenine Ajagappane <Lenine.Ajagappane@amd.com>

Merge branch 'users/lajagapp/add-test-helper' of https://github.com/R…

87c2702

…OCm/TheRock into users/lajagapp/add-test-helper

Update calculate_statistics()

e4bd15c

Signed-off-by: Lenine Ajagappane <Lenine.Ajagappane@amd.com>

lajagapp requested a review from geomin12 March 9, 2026 17:56

geomin12 approved these changes Mar 9, 2026

View reviewed changes

lajagapp merged commit c6bc478 into main Mar 10, 2026
108 of 115 checks passed

lajagapp deleted the users/lajagapp/add-test-helper branch March 10, 2026 01:09

github-project-automation Bot moved this from TODO to Done in TheRock Triage Mar 10, 2026

	"""Initialize common test infrastructure.
	"""Initialize common extended test infrastructure.



		class ExtendedTestBase:
		"""Base class providing shared infrastructure for benchmark and functional tests."""

	Builds the base result structure used by both benchmark and functional tests.
	Builds the base result structure used by extended tests.

	### From Test Base Classes
	### From Extended Test Base Classes

Conversation

lajagapp commented Feb 28, 2026

Summary

Changes

New: utils/extended_test_base.py

Updated: benchmark/scripts/benchmark_base.py

Updated: benchmark/scripts/test_rccl_benchmark.py

Updated: utils/__init__.py, README.md, utils/README.md

Inheritance Hierarchy

Follow-up

Uh oh!

geomin12 left a comment

Choose a reason for hiding this comment

Uh oh!

geomin12 left a comment

Choose a reason for hiding this comment

Uh oh!

geomin12 left a comment

Choose a reason for hiding this comment

Uh oh!

lajagapp commented Mar 5, 2026

Uh oh!

geomin12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geomin12 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

New: `utils/extended_test_base.py`

Updated: `benchmark/scripts/benchmark_base.py`

Updated: `benchmark/scripts/test_rccl_benchmark.py`

Updated: `utils/init.py`, `README.md`, `utils/README.md`