Repository navigation
Cleanups for OBJ S3 - #2356
Cleanups for OBJ S3#2356
Conversation
|
👋 Hi aranadive! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
|
/build |
|
🤖 CI Triage Agent — TL;DR: The container build fails compiling Full analysisSummary: Stages "Build image" (x86_64 node 193 and aarch64 node 208) failed at Dockerfile STEP 68/74 ( Root cause: This file is only compiled when Implicated commit: File: Suggested fix: Add plugins_test_exe = executable('plugins_gtest',
sources : plugins_gtest_sources,
include_directories: [nixl_inc_dirs, utils_inc_dirs, plugins_inc_dirs, '.'],
cpp_args : plugins_gtest_cpp_flags,
dependencies : [nixl_dep, gtest_dep, gmock_dep, absl_strings_dep, absl_time_dep,
plugin_deps, obj_backend_interface] + plugins_gtest_cuda_deps,
link_with: [nixl_build_lib],
install : true
)
Related: PR #2356 (this build), PR #2241 (origin of the gmock-based assertion); no existing issue found for this error signature.
|
|
/build |
|
🤖 CI Triage Agent — TL;DR: The build and image push succeeded; the job failed in the "Allocate DL EP Environment" stage because Full analysisSummary: Stage "Allocate DL EP Environment" failed after 60 minutes with Root cause: The pipeline requests an exclusive single-node allocation on the DL cluster: Slurm accepted it ( The one-hour gap in the log is the intended blocking Secondary issue visible in the cleanup stage: Implicated commit: none — no code commit is implicated. The build of [REDACTED:Hex High Entropy String] compiled and installed cleanly; the failure is in the Slurm allocation step. Nearest related CI work is File: Jenkins shared library Suggested fix:
Related: PR #2356 (this build's PR — unaffected by the failure); #2339 / #2182 (NirWolfer, prior partition-specific GPU pipeline and node-exclusion changes) are the closest precedent for adjusting partition allocation behaviour. No existing issue tracks
|
|
/ok to test 7e33ca9 |
|
/build |
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Root cause: Pure infra failure in the slurm allocation helper, not in nixl code:
The gap is explained by Implicated commit: none — the failure is in the Jenkins File: not a repo file — pipeline stage "Allocate Environment" (stage ID 164), Suggested fix:
Note: the stage ran under Related: none found (issue/PR search for this error signature returned no results)
|
|
🤖 CI Triage Agent — TL;DR: The "Test CPP" stage of one build matrix variant failed because 42 gtest cases that passed exited with code 42 — the gtest harness treats any WARNING log as a test failure, and Full analysisSummary: Stage 496 ("Test CPP", one of six parallel matrix variants) failed with Root cause: Every failing test log shows the same pattern and nothing else: Two pieces combine:
So on any agent where the invariant TSC is unavailable, every test that constructs a This failure is unrelated to the PR's content (obj/2241 review cleanups); nothing in the failing output touches the OBJ plugin. Implicated commit: File: Suggested fix: Preferred — demote the message so it can't fail tests, since it describes a capability fallback rather than a fault: if (!nixlTime::fastClockUsesHwCounter()) {
NIXL_INFO << "telemetry: no invariant CPU counter available; per-transfer timing "
"falls back to steady_clock (higher overhead)";
}and make it fire once per process (e.g. a Related: #1890 (introduced the CPU-counter stopwatch and this warning), #1288 (gtest fails on unexpected warning/error), #1521 ("Fix IB warning test failure in CI" — same class of problem, fixed by ignoring an environment-dependent warning) |
…ynamo#2241) - Resolve an empty accel `type` to "s3" inside objAccelEngineRegistry (has/lookupOrThrow) instead of in createAccelEngine, so every lookup path treats "no type" as the standard engine. - Take custom params as `const nixl_b_params_t *` in the engine_utils getters and createAccelEngine. - cuobj gtest: skip the accelerated tests when NIXL_OBJ_ENDPOINT_OVERRIDE is unset, and fail (rather than skip) when engine construction errors for any reason other than a missing fast path. Factor the env var name into a constant. - Clarify comments on rdmaEngReady() and signV4(). Signed-off-by: Adit Ranadive <aranadive@nvidia.com>
…o#2241) s3_accel/client.h included object/rdma/rdma.h, and s3_accel/engine_impl.h included s3_accel/client.h, so every TU that pulled in either header also compiled the full cuObject/RDMA header set. That inflated per-TU memory enough to OOM the wheel build. awsS3AccelClient only holds a raw pointer and a shared_ptr to the RDMA types, so forward-declare SharedCuObjClient and S3RdmaControlPlane in client.h and include rdma.h from client.cpp instead. Drop the unused s3_accel/client.h include from engine_impl.h and include it directly in engine_impl.cpp, which is the only place that needs the full type. Signed-off-by: Adit Ranadive <aranadive@nvidia.com>
obj_cuobj_plugin.cpp now uses ASSERT_THAT/testing::HasSubstr, so the plugins_gtest target needs gmock_dep; without it the container build fails with 'gmock/gmock.h: No such file or directory'. Signed-off-by: Adit Ranadive <aranadive@nvidia.com>
Replace the "s3" literal used for the default accelerated engine with default_accel_engine_type in engine_utils.h, and use it where the type is registered (s3_accel engine), resolved (registry), and checked (isGenericAccelRequested). Also fix the registration comment, which still said obj_backend normalizes a missing type; the registry does that now. Signed-off-by: Adit Ranadive <aranadive@nvidia.com>
7e33ca9 to
5deb70a
Compare
|
/build |
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Stage 538 ( Root cause: Every one of the 42 failures passed its assertions — the log shows Exit code 42 comes from That predicate is computed in This is unrelated to the PR under test (obj/S3 review cleanups); nothing in the build or the OBJ plugin is implicated. It's a latent environment-sensitive test-harness bug. Implicated commit: 355fbd6 — "telemetry: measure per-transfer durations with a CPU-counter stopwatch (#1890)", e-eygin (added both the calibration logic and the File: Suggested fix: The message is purely informational — a slower-but-correct timing path, not a problem — so it should not be a WARNING. Preferred: demote it to If the WARNING severity must be kept, add an ignore guard alongside the existing const std::regex fastclock_regex(
"telemetry: no invariant CPU counter available");
ligs.emplace_back(fastclock_regex);Mitigation note: this will keep flapping unpredictably across the non-GPU matrix until fixed, since it depends on which agent the leg lands on. Related: PR #2356 (this build); PR #1890 (introduced the stopwatch and the warning); PR #1288 (made unexpected warnings fail the suite); the |
What?
Two follow-ups to #2241 (generic S3-over-RDMA in the OBJ plugin):
s3_accel/client.hto fix a wheel-build OOM.Why?
s3_accel/client.hincludedobject/rdma/rdma.h, so every file including it compiled the full cuObject header set. That pushed compiler memory high enough to OOM the wheel build.How?
client.hforward-declares the RDMA types;rdma.his included only in the.cppfiles that need it.type→"s3"is now resolved in the engine registry instead ofcreateAccelEngine.NIXL_OBJ_ENDPOINT_OVERRIDEis unset (like the Dell tests) and only skip a construction failure when the fast path is unavailable; anything else fails.const nixl_b_params_t *on the param getters, plus comment fixes.Summary by CodeRabbit
New Features
Bug Fixes