util: add reusable comma-separated env-list parser (NIX-1621) - #1927
Conversation
Extract the repeated absl::StrSplit(',') + StripAsciiWhitespace +
skip-empty idiom into a header-only nixl::str::splitStripped helper and
use it in resolveTraceBackends (NIXL_TRACE_BACKENDS). Behavior-preserving;
other open-coded call sites can adopt it as their PRs land.
Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
|
👋 Hi e-eygin! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds reusable stripped string-splitting helpers, uses ChangesTrace backend parsing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/common/str_util.h`:
- Around line 17-18: Update the header guard in str_util.h from
NIXL_UTILS_COMMON_STR_UTIL_H to the full path-derived form
NIXL_SRC_UTILS_COMMON_STR_UTIL_H, and use the same corrected macro in both the
`#ifndef` and `#define` directives.
- Around line 29-30: Add a concise Doxygen-style block comment immediately
before the public splitStripped function, documenting its purpose, input string,
delimiter behavior, and returned stripped segments. Keep the existing signature
and implementation unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: ab486496-c6b2-4b01-803e-badd78959c04
📒 Files selected for processing (5)
src/core/nixl_agent.cppsrc/utils/common/str_util.htest/gtest/unit/meson.buildtest/gtest/unit/util/meson.buildtest/gtest/unit/util/str_util_test.cpp
Match the repo's path-derived header-guard convention (NIXL_SRC_UTILS_COMMON_STR_UTIL_H) and add a Doxygen block on the public splitStripped helper. Addresses CodeRabbit review on ai-dynamo#1927. Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
|
/build |
|
/ok to test 460cb17 |
Per review (ColinNV on ai-dynamo#1927): several env-list consumers split *and* deduplicate, so add a set-returning nixl::str::splitStrippedSet alongside splitStripped and use it in resolveTraceBackends (drops the manual set-insert). Deduped call sites can now parse in one call. Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
|
/build |
|
/ok to test 286bc4d |
|
🤖 CI Triage Agent — TL;DR: The "Run Nixlbench tests" stage was killed (exit 143) after ~48 minutes of total silence following the UCCL Full analysisSummary: Jenkins Root cause: Scanning the log timestamps, the last meaningful application output is Implicated commit: Not the PR under test — PR #1927 does not touch UCCL or the test script. The hang is in the UCCL backend, last substantively changed by File: Suggested fix:
Related: UCCL backend PRs #1428, #1271, #1151; consistency-check PR #1103. No existing open issue matches this specific hang.
|
3 participants
What?
Adds a small, header-only helper
nixl::str::splitStripped(absl::string_view, char delim = ',')in
src/utils/common/str_util.hthat splits a delimited string into non-empty,whitespace-stripped tokens (order-preserving, duplicates kept). Refactors
resolveTraceBackends()insrc/core/nixl_agent.cpp(NIXL_TRACE_BACKENDS) to use it,replacing the open-coded
absl::StrSplit(',')+absl::StripAsciiWhitespace+ skip-emptyloop with a range-insert into the backend set. Adds unit tests
(
test/gtest/unit/util/str_util_test.cpp).Why?
Follow-up from the PR #1897 review (NIX-1621): the same comma-separated env-list parsing
idiom is open-coded in several places (tracing backends, DOCA exporter backends, histogram
buckets, per-metric activation). This lands the shared helper and converts the one call site
that exists on
maintoday (resolveTraceBackends). Kept intentionally minimal andbehavior-preserving so the remaining call sites can adopt it independently as their PRs land;
if this merges first the helper is already available to cherry-pick on rebase, avoiding a
separate follow-up PR.
Ref: https://linear.app/nvidia/issue/NIX-1621
How?
splitStrippedcentralizes theStrSplit+StripAsciiWhitespace+ skip-empty logic andreturns a
std::vector<std::string>; deduplication stays a caller concern (resolveTraceBackendsstill collects into a
std::set, so the set-but-empty "off" semantics and nsys auto-enableprecedence are unchanged). No
TELEMETRY_VERSION/API change. Existing tracing tests(
Tracing.ResolveBackends*) plus 4 newStrSplitStrippedcases pass.Summary by CodeRabbit