Skip to content

Conversation

subodh-dubey-amd
Copy link

@subodh-dubey-amd subodh-dubey-amd commented Sep 25, 2025

Motivation

Add hipsolver & rocsolver to the ROC test suite to ensure comprehensive testing coverage for the ROCm linear algebra solver library and catch regressions early in the CI pipeline.

Technical Details

  • Added test_hipsolver.py & test_rocsolver.py script in test_executable_scripts
  • Updated fetch_test_configurations.py to include hipsolver & rocsolver in the test matrix
  • Integrated with existing GitHub Actions workflow following the same pattern as other BLAS libraries

Test Plan

  • Verified hipsolver test script executes correctly with hipsolver-test binary
  • Verified rocsolver test script executes correctly with rocsolver-test binary
  • Confirmed test matrix generation includes hipsolver tests
  • Confirmed test matrix generation includes rocsolver tests
  • Validated GTest sharding works for parallel execution

Test Result

hipsolver & rocsolver tests successfully integrated into CI pipeline with proper sharding and platform support.

Submission Checklist

Copy link
Member

@marbre marbre left a comment

Choose a reason for hiding this comment

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

This seems to add a configuration for hipSOLVER and not rocSOLVER. Please update the PR title and description accordingly. Furthermore, pre-commit issues need to be addressed.

"timeout_minutes": 60,
"test_script": f"python {_get_script_path('test_hipsolver.py')}",
"platform": ["linux", "windows"],
"total_shards": 4,
Copy link
Contributor

Choose a reason for hiding this comment

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

based on the test logs, the tests take about 2 seconds per shard to run. Since the test time is so little, we don't need 4 shards. Better to do just 1

"hipsolver": {
"job_name": "hipsolver",
"fetch_artifact_args": "--blas --tests",
"timeout_minutes": 60,
Copy link
Contributor

Choose a reason for hiding this comment

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

can you adjust the timeout minutes to correctly reflect the time tests take?


logging.basicConfig(level=logging.INFO)

cmd = [f"{THEROCK_BIN_DIR}/hipsolver-test", "--gtest_filter=*float_complex*-*known_bug*"]
Copy link
Contributor

Choose a reason for hiding this comment

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

is this filter correct? I would reach out to SOLVER team to make sure this matches "presubmit"!

Copy link
Contributor

@geomin12 geomin12 left a comment

Choose a reason for hiding this comment

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

please update the title to reflect the correct tests added as Marius mentioned! It looks good and passes for Linux! The windows build failed, but I am retrying that.

If windows and linux both pass for tests, this should be good to go.

@subodh-dubey-amd subodh-dubey-amd changed the title rocsolver Test for the ROCK hipsolver & rocsolver Test for the ROCK Sep 26, 2025
@geomin12
Copy link
Contributor

looks like tests are failing. we could enable these tests and add gtest ignore filters and link to these issues:

[ci] SOLVER windows test errors for gfx1151 · Issue #1080 · ROCm/TheRock / [SWDEV-555871] [gfx1151/gfx942] [SOLVER] rocSOLVER and hipSOLVER test failures - INTERNAL

"timeout_minutes": 120,
"test_script": f"python {_get_script_path('test_rocsolver.py')}",
"platform": ["linux"],
"total_shards": 4,
Copy link
Member

Choose a reason for hiding this comment

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

The newly added rocsolver tests (I think this was hipsolver only when I last revied) still use 4 shards and have a 120 minute timeout. Is this needed? Otherwise please set lower values as for hipsolver. If a workflow hangs, it will otherwise block resources for 2 hours!

sources/
.vscode/
*.venv
*venv
Copy link
Member

Choose a reason for hiding this comment

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

This is a reasonable change (had the same in mind earlier!) but should not be part of this PR. Please send a separate patch.

/install/
/output-*/
/out/
therock-build/*
Copy link
Member

Choose a reason for hiding this comment

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

As above. If desired (most folks use build), please send a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: TODO
Development

Successfully merging this pull request may close these issues.

3 participants