Skip to content

[TEMP][DO NOT REVIEW] Diagnose PR 11685 Azure protocol selection - #11858

Closed
jeffnvidia wants to merge 13 commits into
openucx:masterfrom
jeffnvidia:diag/pr11685-azure-selection
Closed

jeffnvidia wants to merge 13 commits into
openucx:masterfrom
jeffnvidia:diag/pr11685-azure-selection

Conversation

@jeffnvidia

Copy link
Copy Markdown
Contributor

Temporary draft used only for an Azure A40 base/PR diagnostic. Please do not review or merge.

The initial head is the exact PR 11685 base plus test-only logging. It does not change protocol selection, test assertions, or PR 11685.

The logging prints the AM protocol ranges and, for each am/rndv range, the remote selection and synthetic rkey system-device inputs. After the control GPU result is captured, this temporary head will be updated to the exact PR 11685 merge plus the identical logging for the comparison. The draft PR and fork branch will then be closed/deleted.

@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

@svc-ucx

svc-ucx commented Aug 30, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Codestyle AUTHORS file update check) · commit 9d82a1f0

TL;DR: The "AUTHORS file update check" job failed because commit [REDACTED:Hex High Entropy String] is authored by jeffnvidia <jmahou@nvidia.com>, who is not listed in the AUTHORS file — the fix is to run ./contrib/authors_update.sh and commit the resulting one-line AUTHORS addition to the PR.

Full analysis

Summary: Azure Pipelines codestyle job "AUTHORS file check" exited with code 1 because git diff --exit-code found an uncommitted AUTHORS change after running the authors-update script.

Root cause: The check computes base..head (8a9c6268..[REDACTED:Hex High Entropy String]), runs contrib/authors_update.sh and requires the working tree to stay clean. The script found a new author for this PR:

Names:
++ jeffnvidia <jmahou@nvidia.com>

Neither the email jmahou@nvidia.com nor the name jeffnvidia matches any existing AUTHORS entry (see the grep -iqw "$email" && grep -iq "$name" test at contrib/authors_update.sh:36), so the script appended jeffnvidia <jmahou@nvidia.com> after line 140 (Zihao Zhao <zizhao@nvidia.com>), producing a diff and failing the gate. This is a legitimate policy failure, not an infrastructure/flake issue — the PR simply does not include the required AUTHORS update for a first-time (or newly-aliased) contributor.

Implicated commit: [REDACTED:Hex High Entropy String] (author: jeffnvidia jmahou@nvidia.com), on branch diag/pr11685-azure-selection

File: AUTHORS:140-141 (insertion point); check logic at contrib/authors_update.sh:36-46

Suggested fix: In the PR branch, run:

./contrib/authors_update.sh origin/master..HEAD
git add AUTHORS && git commit -m "AUTHORS: add jeffnvidia"

and push. Alternatively, if this contributor is already in AUTHORS under a different display name/email (e.g. a real name rather than the jeffnvidia GitHub handle), fix the commit's author identity instead — set git config user.name/user.email to the already-listed identity and amend/rebase the commit — so the script's name/email match at authors_update.sh:36 succeeds and no AUTHORS change is needed. Note the sort is LC_COLLATE=C, so lowercase names correctly sort after uppercase ones; keep the generated ordering as-is rather than hand-editing.

Related: PR #11858 (this PR); no existing issue found for this AUTHORS entry.

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 812a3b3c-4e07-466f-855e-cfcdb00eae43 in the triage console for the audit trail.

}

protected:
void dump_am_rndv_selection(const ucp_proto_select_elem_t *select_elem)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The entire dump_am_rndv_selection() helper is debug scaffolding. Every message is prefixed with PR11685_DIAG, which is a PR/issue tracking tag, not a real diagnostic. This dumps internal selection state on every select_am_rndv_remote_proto_config() call and adds nothing to test coverage — it makes no assertions. Please remove it before merge.

return nullptr;
}

dump_am_rndv_selection(select_elem);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The invocation of dump_am_rndv_selection() is debug scaffolding and should be removed before merge.

am_explicit_scheme_disables_force_provenance,
"RNDV_PIPELINE_SHM_CUDA_STAGING_FORCE=y",
"RNDV_SCHEME=get_zcopy")
"RNDV_SCHEME=get_zcopy",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Adding "PROTO_INFO=y" to am_explicit_scheme_disables_force_provenance only turns on extra protocol-info printing for debugging; the test body is unchanged and asserts nothing new. Please revert this config addition.

@svc-nvidia-pr-review

Copy link
Copy Markdown

The PR11685_DIAG prefix in the log messages references PR #11685 while this is PR #11858, which reinforces that this is leftover debug output that should not land.

If any part of this is meant to become a permanent regression check (for example verifying remote_select_param->op_id_flags or rkey_cfg_index values), it should be converted into real EXPECT_* assertions rather than UCS_TEST_MESSAGE dumps, and the tracking-tag prefixes dropped.

@jeffnvidia
jeffnvidia force-pushed the diag/pr11685-azure-selection branch from 9d82a1f to cc5c691 Compare August 30, 2026 14:50
@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

"RNDV_PIPELINE_SHM_CUDA_STAGING_FORCE=y",
"RNDV_SCHEME=get_zcopy")
"RNDV_SCHEME=get_zcopy",
"PROTO_INFO=y")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

pls drop PROTO_INFO=y here; the test does not consume the protocol-info dump, this looks like leftover debugging that came in with the PR11685_DIAG dump. Suggested change: "RNDV_SCHEME=get_zcopy")

@jeffnvidia jeffnvidia closed this Aug 30, 2026
@svc-ucx

svc-ucx commented Aug 30, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Tests gpu on worker 0) · commit cc5c6916

TL;DR: The GPU gtest job failed on a single test, shm_ib/test_ucp_proto_am_rndv.am_explicit_scheme_disables_force_provenance/0, which hard-asserts that am/rndv is the protocol chosen for a 1 MB CUDA AM send while running with UCX_RNDV_SCHEME=get_zcopy; with an explicit RNDV scheme all non-matching rndv variants report UCS_MEMUNITS_INF, so on this shm,ib GPU worker no am/rndv variant is selected and the unguarded EXPECT_STREQ("am/rndv", ...) / ASSERT_NE(nullptr, remote_proto_config) fires. Fix by pinning PROTOS (as the RMA fixture does) or skipping when am/rndv isn't selected, instead of hard-asserting.

Full analysis

Summary: make test in "Tests gpu on worker 0" exited 2 — 10228 tests ran, 10207 passed, 1 failed: shm_ib/test_ucp_proto_am_rndv.am_explicit_scheme_disables_force_provenance/0 (GetParam() = shm,ib,cuda_copy,rocm_copy); no crash, timeout or hang (test finished in ~51 min with continuous output, last stage exited cleanly).

Root cause: The failing test (test/gtest/ucp/test_ucp_proto.cc:858) runs with RNDV_PIPELINE_SHM_CUDA_STAGING_FORCE=y, RNDV_SCHEME=get_zcopy, PROTO_INFO=y and then calls check_am_rndv_remote_proto_config(0, 0), whose helper select_am_rndv_remote_proto_config() (line 481-526) looks up UCP_OP_ID_AM_SEND with CUDA memory at 1 MB and requires the winning protocol to be named am/rndv (EXPECT_STREQ at line 518, followed by ASSERT_NE(nullptr, remote_proto_config) at line 534). Setting an explicit RNDV_SCHEME takes rndv_mode out of UCP_RNDV_MODE_AUTO, which (a) disables the force policy entirely (ucp_proto_rndv_shm_pipeline_force_enabled(), src/ucp/rndv/proto_rndv.inl:20-28) and (b) makes ucp_proto_rndv_cfg_thresh() return UCS_MEMUNITS_INF for every rndv variant that does not match the requested scheme (proto_rndv.inl:131-150, consumed in proto_rndv.c:479-493). On this shm,ib+cuda_copy/rocm_copy GPU configuration the get_zcopy remote receive variant for a CUDA source buffer is not viable, so all am/rndv variants end up at INF and a non-rndv (eager) protocol wins at 1 MB — the test has no guard for that, unlike its RMA sibling test_ucp_proto_rma_rndv, which pins modify_config("PROTOS", "put/rndv,get/rndv,rndv/*") (line 315) and therefore passes in the same run. Note the exact EXPECT text was above the retrievable tail of the Azure log (only the gtest summary and the [ FAILED ] line survive), so the failing line is inferred from the test's own assertion structure plus the surviving evidence that only this AM+explicit-scheme variant failed while am_default_remote_model_preserves_legacy_key and am_force_remote_model_remains_untagged (identical except for RNDV_SCHEME) passed.

Implicated commit: cc5c691 (jeffnvidia, "TEST/UCP: Add temporary PR 11685 selection diagnostics") is the head under test and owns the current form of these AM tests; the underlying selection behaviour comes from the CUDA-staging-force series b34ac9a / 9cc0808 (Nathan Bellalou, PR #11685/#11641) and the test-guard commit [REDACTED:Hex High Entropy String] (Tomer Gilad) which added environment skips only for the rma_rndv tests.

File: test/gtest/ucp/test_ucp_proto.cc:858-867 (test body), test/gtest/ucp/test_ucp_proto.cc:518 and :534 (failing expectations); behaviour in src/ucp/rndv/proto_rndv.inl:20-28,131-150 and src/ucp/rndv/proto_rndv.c:479-493

Suggested fix: Make test_ucp_proto_am_rndv deterministic the same way the RMA fixture is: add an init() override with modify_config("PROTOS", "am/rndv,rndv/*") (and/or RNDV_THRESH=0) so the rndv RTS is guaranteed to win for the 1 MB CUDA send, and turn the protocol-name check into a graceful skip (UCS_TEST_SKIP_R("am/rndv not selected under explicit RNDV_SCHEME")) instead of EXPECT_STREQ + ASSERT_NE(nullptr, ...) when the selected protocol isn't am/rndv. If the intent is purely to verify that an explicit RNDV_SCHEME clears the force provenance, assert only on count_rkey_configs_with_flag(worker(), UCP_RKEY_CONFIG_FLAG_PROTO_ESTIMATION) == 0, which does not depend on which protocol wins. Re-run with the PR11685_DIAG dump from this build to confirm which threshold/protocol was actually chosen before finalizing.

Related: PR #11858 (this diagnostic branch); upstream feature PRs #11685 / #11641 (intra-node CUDA staging force), guard precedent in commit [REDACTED:Hex High Entropy String] for test_ucp_proto_rma_rndv.

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 83382450-8405-4447-8c41-8ec6c5615b3c in the triage console for the audit trail.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants