Repository navigation
[TEMP][DO NOT REVIEW] Isolate PR 11685 mock topology state - #11861
jeffnvidia wants to merge 14 commits into
Conversation
|
🤖 Starting review — findings will be posted here when done. |
| } | ||
|
|
||
| protected: | ||
| void dump_am_rndv_selection(const ucp_proto_select_elem_t *select_elem) |
There was a problem hiding this comment.
blocker — leftover debug/diagnostic code must be removed. The dump_am_rndv_selection() function and its call at line 509 are pure debug scaffolding: they contain no assertions and only emit UCS_TEST_MESSAGE lines tagged PR11685_DIAG. This debug output: adds ~75 lines of test-diagnostic noise with no verification value; references the wrong PR number (PR11685 in PR #11861); spams the test log on every test_ucp_proto_am_rndv run. Remove the entire dump_am_rndv_selection function (lines 403–479) and its call at line 509.
| return nullptr; | ||
| } | ||
|
|
||
| dump_am_rndv_selection(select_elem); |
There was a problem hiding this comment.
Remove the call to dump_am_rndv_selection() — part of the debug scaffolding that must be removed before merge.
| "RNDV_PIPELINE_SHM_CUDA_STAGING_FORCE=y", | ||
| "RNDV_SCHEME=get_zcopy") | ||
| "RNDV_SCHEME=get_zcopy", | ||
| "PROTO_INFO=y") |
There was a problem hiding this comment.
PROTO_INFO=y looks like a leftover debug flag. am_explicit_scheme_disables_force_provenance only asserts on check_am_rndv_remote_proto_config(0, 0) (op_id_flags and rkey config flag counts). PROTO_INFO=y just dumps protocol info at init and does not affect any assertion. It appears to be debug leftover paired with the PR11685_DIAG dump. Please drop it unless it is actually needed.
| * device while estimating the protocol that the peer will select. | ||
| * The peer will use its actual local system device when selecting the | ||
| * protocol. | ||
| */ |
There was a problem hiding this comment.
Non-blocking (core fix): Setting mem_info.sys_dev = UCS_SYS_DEVICE_ID_UNKNOWN for the remote-proto select param is sound — the rkey sys_dev is in the remote topology namespace and must not be used as a local device when estimating the peer's protocol. The stored proto_config->select_param is taken from the outer select_param_copy in ucp_proto_select_elem_init, so this change only affects lane modeling during probe, not the selection hash key.
| ucp_proto_select_param_t select_param; | ||
| } ucp_proto_rndv_mtype_init_params_t; | ||
|
|
||
| static UCS_F_ALWAYS_INLINE void ucp_proto_rndv_mtype_init_params_prepare( |
There was a problem hiding this comment.
Non-blocking: Overriding select_param.sys_dev with the staging (frag_mem_info->sys_dev) device is the necessary companion fix. With the rkey sys_dev now UNKNOWN, the mtype probe passes frag_mem_info as reg_mem_info, and ucp_proto_common_get_lane_perf/ucp_proto_common_filter_min_frag assert reg_mem_info.sys_dev == select_param->sys_dev under SEND_ZCOPY. This override keeps that invariant. The modified select_param is not propagated into the stored lookup key, as the code comment states.
| setup_progress_mock(sender().worker(), m_progress_mock); | ||
| setup_progress_mock(receiver().worker(), m_progress_mock); | ||
|
|
||
| if (mode == rndv_mode::put_ppln) { |
There was a problem hiding this comment.
Non-blocking: The put_ppln pre-check flow is correct: the first smoke_test(true) runs before mock setup; the new pre-check runs with mocks installed but m_fail_target_proto=false, so close_peer() and op replacement are skipped and only m_target_proto_seen is set. m_is_peer_closed stays false for the real run. This addresses the fact that the capability check in init() doesn't cover staging-device reachability.
| Jakir Kham <jakirkham@gmail.com> | ||
| Jason Gunthorpe <jgg@mellanox.com> | ||
| Jeff Daily <jeff.daily@amd.com> | ||
| Jeffrey Mahou <jmahou@nvidia.com> |
There was a problem hiding this comment.
Non-blocking: Entry is correctly alphabetically ordered.
|
🤖 CI Triage Agent — TL;DR: The gtest run failed on a single timing-sensitive test, Full analysisSummary: Root cause: Evidence from the log: the run completed normally (
So the assertion measures disk I/O plus environment scanning against a 50 ms budget, with no averaging and no retry: Implicated commit: unknown — pre-existing flakiness; File: test/gtest/ucs/test_config.cc:792 ( Suggested fix: Make the assertion robust instead of raising the wall-clock limit blindly:
This failure should not block PR #11861; re-run the job to confirm it is green. Related: #2292 (previous increase of this exact test's time limit), #2131 (
|
e8b9555 to
900fc70
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
|
||
| set_gtest_common_test_flags() { | ||
| export GTEST_RANDOM_SEED=0 | ||
| export GTEST_RANDOM_SEED=37290 |
There was a problem hiding this comment.
minor — Only set_gtest_common_test_flags is switched to the fixed seed 37290; run_gtest_release still exports GTEST_RANDOM_SEED=0. Also, the specific value has no explanation. Consider a brief comment on why a fixed seed is needed (reproducing the ordering-dependent failure this PR addresses), and whether run_gtest_release should use the same seed for consistency.
Suggestion:
export GTEST_RANDOM_SEED=37290 # fixed non-zero seed for reproducible shuffle
900fc70 to
f0e7095
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
|
||
| set_gtest_common_test_flags() { | ||
| export GTEST_RANDOM_SEED=0 | ||
| export GTEST_RANDOM_SEED=37290 |
There was a problem hiding this comment.
why hardcode the gtest random seed here? this changes the shuffle order for all gtests and looks unrelated to the rndv sys_dev fix. also run_gtest_release still uses GTEST_RANDOM_SEED=0, so the seed policy is now inconsistent. can this go in a separate PR, or is a specific seed needed to reproduce the fixed bug? Additionally, -s 11689 only reaches run_gtest_make (not run_specific_tests, which calls set_gtest_common_test_flags without GTEST_EXTRA_ARGS).
|
🤖 CI Triage Agent — TL;DR: The only failing test in the whole 8532-test ASan run is the newly added Full analysisSummary: Root cause: The test is a resource-usage regression test, not a functional one. It performs 40 000
Under AddressSanitizer neither budget is realistic: every interval-tree node allocation carries redzones plus a retained malloc stack trace, freed nodes go into ASan's quarantine instead of back to the OS, and ASan's allocator never trims — 40 000 insert/merge/free cycles inflate Implicated commit: [REDACTED:Hex High Entropy String] (PR #11861, branch File: test/gtest/ucs/test_rcache.cc:1100-1138 (thresholds at lines 1106-1107, failing checks at lines 1124 and 1130); related product code src/ucs/memory/rcache.c:636-650 Suggested fix:
Related: upstream interval-tree work this test guards: 311e350 (#11248 "UCS/RCACHE: Change inv_q type to interval tree"), c0b9ee3 (#11213 "Interval tree fixes"), 6b25652 (#11120 "Introduce interval tree data structure"); no existing issue/PR mentions
|
Temporary CI diagnostic for PR #11685.
This branch keeps the exact #11685 merge production code unchanged and retains the previously validated selection diagnostics. It removes only the two mock tests newly added by #11685, restoring test_ucp_proto_mock.cc exactly to the base revision, while leaving the original failing AM RNDV test unchanged.
Purpose: determine whether the Azure failure is caused by production protocol selection or by topology/mock state leaking from the new tests into a later randomized test.
This PR is not intended for review or merge. It will be closed and the temporary branch deleted after the Azure result is captured.