Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe Jenkins test script changes SLURM job-ID retrieval and performance-directory discovery. The job-ID read no longer redirects stderr remotely. Directory results are filtered locally for ChangesJenkins test shell updates
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly identifies the infrastructure change: collecting performance result folders on Slurm clusters with csh login shells. |
| Description check | |
The description explains the issue, the shell-related cause, the fix, test coverage, and expected end-to-end validation. It follows the required template and clearly identifies that end-to-end validat… |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Description check
-
Explanation
The description explains the issue, the shell-related cause, the fix, test coverage, and expected end-to-end validation. It follows the required template and clearly identifies that end-to-end validation remains recommended.
Full details: Docstring Coverage
-
Explanation
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
| @@ -2615,7 +2617,11 @@ def runLLMTestlistWithSbatch(pipeline, platform, testList, config=VANILLA_CONFIG | |||
| def scpXmlCmd = scpFromRemoteCmd(remote, "${remoteWorkspaceTrk}/results*.xml", "${stageName}/") | |||
| def scpUnfinishedCmd = scpFromRemoteCmd(remote, "${remoteWorkspaceTrk}/unfinished_test.txt", "${stageName}/") | |||
| def sshRefreshCacheCmd = Utils.sshUserCmd(remote, "\"ls '${remoteWorkspaceTrk}/' > /dev/null 2>&1; ls -la '${remoteWorkspaceTrk}/results.xml' > /dev/null 2>&1 || true\"") | |||
There was a problem hiding this comment.
This command still uses Bash redirection and fails under csh.
|
Could we validate this patched Jenkins pipeline on oci-jhb or oci-aga and link a GB300 Disagg-PerfSanity tarball containing disagg_upload-* and cache_transceiver_precheck/? Current checks do not exercise that collection path |
|
Automatically added "ci: full pre-merge approved" because this PR has satisfied the required GitHub review approvals. Unresolved review conversations and other required checks remain independent merge requirements. |
…ts (NVIDIA#20261) PerfSanity stages on oci-jhb and oci-aga (the GB300 stages) do not upload their perf folders, the per-test aggr_upload-*/disagg_upload-* folders with the server logs and cache_transceiver_precheck data. Nothing is logged. Since NVIDIA#14138 the sbatch path lists these folders with find '<ws>' ... -name 'aggr*' -o -name 'disagg*' ... -print 2>/dev/null Both consumers paste that command between single quotes, which strips its inner single quotes, and sshd runs it under svc_tensorrt's login shell. On oci-jhb and oci-aga that shell is csh. (oci-hsg also uses csh, but its sshd ForceCommand runs the command with bash.) Under csh the bare globs fail with "No match", and 2>/dev/null would become an extra argument plus a stdout redirect. The error is discarded locally, so the list comes back empty. Run only "find <ws> -mindepth 1 -maxdepth 1 -type d" remotely and keep the aggr/disagg folders with a local grep. Move the NFS cache-refresh command's redirects to the local side as well; under csh they wrote a stray file named 1 into svc_tensorrt's home. Also drop 2>/dev/null from the slurm_job_id.txt fallback read, which under csh always returned an empty job ID. Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com> (cherry picked from commit 5828d4f)
0f1f4aa to
89e5349
Compare
Dev Engineer Review
jenkins/L0_Test.groovynow runsfindremotely to list top-level directories, then filters the results locally for/aggrand/disagg. This avoids csh glob and redirection behavior that could prevent performance folders from being collected. Theslurm_job_id.txtfallback read no longer uses2>/dev/null, which could suppress stdout under csh. No current review findings or test results were supplied.QA Engineer Review
No test changes.
Per-File QA Perspective
jenkins/L0_Test.groovy: Verify that the progress watcher collectsaggr*anddisagg*folders and that the fallback read retrieves the SLURM job ID when the file exists. Validation onoci-jhboroci-agaremains a follow-up; the supplied context does not report it as complete.Description
Cherry-pick of #20261 to release/1.3. The patch applies unchanged; this branch has the same perf-folder listing code as main.
PerfSanity stages on oci-jhb and oci-aga (the GB300 stages) lose their per-test
aggr_upload-*/disagg_upload-*folders: server logs andcache_transceiver_precheck/data. The folder-listingfindloses its quotes when the sh blocks paste it between single quotes, and svc_tensorrt's login shell on those clusters is/bin/csh. There the bare globs fail withNo match, and2>/dev/nullredirects stdout. The listing comes back empty with no error. For example, every GB300 Disagg-PerfSanity tarball in release-1.3 L0_PostMerge #20–#28 has no perf folder.The fix runs only
find <ws> -mindepth 1 -maxdepth 1 -type dremotely and keeps theaggr*/disagg*folders with a localgrep. It also drops the csh-unsafe2>/dev/nullfrom theslurm_job_id.txtfallback read. See #20261 for the full analysis and the per-cluster data.Test Coverage
Same as #20261:
bsd-cshandbashas the remote shell. The new command lists theaggr_*/disagg_*folders under both shells; the old one lists nothing under csh.pre-commitpasses.To check end to end, run a GB300
Disagg-PerfSanitystage that lands on oci-jhb or oci-aga. Its tarball should then contain thedisagg_upload-*folder.PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.🤖 Generated with Claude Code