Skip to content

fix(rocSHMEM): stale signal_ops_on_stream test ID; register missing QP/SDMA functional tests, #AIROCSHMEM-559 - #14946

Merged
amirsojoodi merged 6 commits into
developfrom
users/asojoodi/rocshmem-fix-functional-tests
Nov 6, 2026
Merged

amirsojoodi merged 6 commits into
developfrom
users/asojoodi/rocshmem-fix-functional-tests

Conversation

@amirsojoodi

Copy link
Copy Markdown
Contributor

Motivation

  • Fix signal_ops_on_stream's test ID in CTestFunctionalTests.cmake and driver.sh: 174 now belongs to QpPutNbiPoll after later enum insertions, so the test was silently running the wrong algorithm and failing with a GDA-backend error.
  • Register the previously-unwired qp_pingpong, qp_putnbi, qp_putnbi_poll, sdma_pingpong, sdma_putnbi, sdma_putnbi_poll functional tests in CTestFunctionalTests.cmake, driver.sh, and test_categories.yaml. Each forces its required backend/config explicitly (ROCSHMEM_BACKEND=gda for QP; ROCSHMEM_BACKEND=ipc+ROCSHMEM_SDMA_ENABLED=1 for SDMA) rather than relying on the default.

Issue Tracking

JIRA ID: AIROCSHMEM-559

Copilot AI balanced review requested due to automatic review settings November 3, 2026 18:55
@amirsojoodi
amirsojoodi requested a review from a team as a code owner November 3, 2026 18:55
@therock-pr-bot

therock-pr-bot Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Unconditional QP and SDMA scheduling can break valid runs on incompatible hardware or multi-node placements, while missing build-option guards let tests pass without executing their intended checks.

4 open issues
What changed in this PR

This PR corrects rocSHMEM functional-test dispatch and connects six existing QP and SDMA tests to the test runners.

Changes:

  • Corrects signal_ops_on_stream from test ID 174 to 195.
  • Registers QP and SDMA tests in CTest and the shell driver, with explicit backend settings.
  • Adds the new test families to weekly test categorization.
File Description
projects/​rocshmem/​tests/​test_categories.yaml Adds weekly categorization patterns for QP and SDMA tests.
projects/​rocshmem/​tests/​functional_tests/​CTestFunctionalTests.cmake Corrects test IDs and registers backend-specific CTest groups.
projects/​rocshmem/​scripts/​functional_tests/​driver.sh Updates test IDs, schedules the six tests, and sets their runtime environments.

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The SDMA tests can inherit an MPI hostfile that places ranks on different nodes, causing IPC initialization to fail unless hostfile launches are guarded or the tests are constrained to one node.

1 open issue
4 resolved since last review

Review effort: Balanced

@math-ci

math-ci Bot commented Nov 4, 2026

Copy link
Copy Markdown

Resource usage report (8gfx942)

Dashboard artifact

@abouteiller abouteiller left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

THe copilot comment is valid otherwise lgtm

@math-ci

math-ci Bot commented Nov 4, 2026

Copy link
Copy Markdown

Resource usage report (8gfx942)

Dashboard artifact

@amirsojoodi
amirsojoodi force-pushed the users/asojoodi/rocshmem-fix-functional-tests branch from cede826 to 27c241b Compare November 4, 2026 19:41
@math-ci

math-ci Bot commented Nov 5, 2026

Copy link
Copy Markdown

Resource usage report (8gfx942)

Dashboard artifact

@math-ci

math-ci Bot commented Nov 5, 2026

Copy link
Copy Markdown

Resource usage report (4gfx1201)

Dashboard artifact

@peymanr

peymanr commented Nov 5, 2026

Copy link
Copy Markdown

PR Pundit review

(Testing PR Pundit Code Review agent. Not all comments may be relevant. This is a one-time automated pass: please address what is useful, no need to request a re-review. Please reply to each point, whether it applies or not: we harvest these replies, and both kinds are enormously helpful for tuning the agent. Reach out to @peymanr for suggestions.)

# Finding Where
1 🟠 SDMA tests report PASSED when SDMA falls back to IPC: this PR skips without skip exit code projects/rocshmem/tests/functional_tests/sdma_ping_pong_tester.cpp:155

1. 🟠 Should fix: SDMA tests report PASSED when SDMA falls back to IPC: this PR skips without skip exit code

projects/rocshmem/tests/functional_tests/sdma_ping_pong_tester.cpp:155

When SDMA connect fails on every local rank and falls back to IPC, SdmaPingPongTester and SdmaPutNbiTester print SKIPPED and skip the kernel, but the process still exits 0. CTest only treats exit code 125 (SKIP_RETURN_CODE) as a skip, so sdma_pingpong/sdma_putnbi/sdma_putnbi_poll report PASSED for a run that exercised nothing. This matches the existing convention (buffer_register_symmetric, device_bitcode also print SKIPPED and exit 0), and the fallback is logged with LOG_ERROR. Exiting 125 would make the skip visible in CTest. Also, sdma_putnbi_poll reuses SdmaPutNbiTester, so its log says sdma_putnbi: SKIPPED.

Who is affected: Weekly/full-tier SDMA functional test runs on nodes where SDMA connect fails (e.g. no xGMI) or on a regression that forces the fallback.

Checked against the code by an independent pass.

Related past reviews: #5268 on projects/rocshmem/scripts/unit_tests/driver.sh, #6334 on projects/rocshmem/tests/functional_tests/CTestFunctionalTests.cmake, #3165 on projects/rocshmem/tests/functional_tests/tester.hpp, #12748 on projects/rccl/test/transport/GinDeviceMPITests.cpp

Shared from a pilot of PR Pundit, an agentic code reviewer that builds and tests the change and reads the project's past reviews. Reviewed at c81e6692; findings checked and posted by @peymanr; points already raised on this PR are left out.

@amirsojoodi
amirsojoodi force-pushed the users/asojoodi/rocshmem-fix-functional-tests branch from c81e669 to fa9a842 Compare November 5, 2026 15:06
@math-ci

math-ci Bot commented Nov 5, 2026

Copy link
Copy Markdown

Resource usage report (4gfx1201)

Dashboard artifact

@math-ci

math-ci Bot commented Nov 5, 2026

Copy link
Copy Markdown

Resource usage report (8gfx942)

Dashboard artifact

@math-ci

math-ci Bot commented Nov 6, 2026

Copy link
Copy Markdown

Resource usage report (8gfx950-bcom-rocev2)

Dashboard artifact

@amirsojoodi
amirsojoodi merged commit 521dfa3 into develop Nov 6, 2026
60 of 63 checks passed
@amirsojoodi
amirsojoodi deleted the users/asojoodi/rocshmem-fix-functional-tests branch November 6, 2026 15:24
@systems-assistant

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants