Repository navigation
Conversation
tanmayv25
changed the title
rmccorm4
reviewed
Mar 15, 2025
| - 'container/deps/requirements.vllm.txt' | ||
| - 'container/deps/vllm/**' | ||
| tensorrtllm: | ||
| - 'container/Dockerfile.tensorrt_llm' |
Contributor
There was a problem hiding this comment.
Can you touch one of these files to confirm the trigger runs the trtllm job and that it passes? Currently triggered pipeline only ran vllm job on gitlab.
tanmayv25
requested review from
alec-flowers,
ishandhanani and
ptarasiewiczNV
as code owners
March 15, 2025 08:22
Contributor
Author
|
@nv-anants Why are these changes not triggering CI for TRTLLM? |
tanmayv25
force-pushed
the
tanmayv-ci-build
branch
from
March 15, 2025 08:30
a09c10e to
789c2fa
Compare
Member
I see trtllm build running as part of latest gitlab CI |
nv-anants
approved these changes
Mar 16, 2025
saturley-hall
force-pushed
the
tanmayv-ci-build
branch
from
April 11, 2025 03:28
0ab63c5 to
1ce30e2
Compare
Contributor
Author
|
Merged |
biswapanda
added a commit
that referenced
this pull request
May 8, 2026
Address findings from a follow-up Codex code review of the PR scope.
network_send_handler (server.rs):
- Replace `assert_ne!(control_msg, ControlMessage::Sentinel, ...)` with a
warn + continue. Sentinel arriving on the send-side control channel is
a producer-side bug, not a process-fatal one.
- Replace `serde_json::to_vec(&control_msg).expect(...)` with a match
that warns and continues on Err. The control enum is small and closed,
so failure is implausible — but Rule 1 says don't bet the process on
it. Logs use structured fields throughout.
wait_for_server_shutdown (client.rs):
- Convert the read-error inspect_err from
`tracing::debug!("failed to read from stream: {:?}", e)` to
`tracing::debug!(err = ?e, "failed to read from stream")`. Same
structured-field treatment as the rest of this file.
process_control_message (server.rs):
- Drop the `TODO(#171) - address fatal errors` and rephrase
`"fatal error - unexpected control message received - this should
never happen"` to `"unexpected control message on response stream"`.
The caller (network_receive_handler) already handles this branch as
a stream-local Kill, not a fatal.
Test comment hygiene:
- Move `tokio::io::{AsyncWriteExt, ReadHalf, WriteHalf}` and
`tokio::net::TcpStream` to the top of `mod tests` (they were
mid-module after a marker comment).
- Remove the panic-replacement banner block; test names self-document.
- Trim two SAFETY (test-only) comments from 6 lines to 2.
- Trim the test_handle_reader_handles_tcp_rst docstring from 6 lines
to 2.
- Trim test_wait_for_server_shutdown_skips_stopped_context docstring
to a single cross-reference line; the production-code comment in
wait_for_server_shutdown carries the full rationale.
Verified: cargo build, cargo test pipeline::network::tcp:: (44 passed),
cargo clippy -- -D warnings, cargo fmt --check all clean.
Signed-off-by: Biswa Panda <biswa.panda@gmail.com>
4 of 5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently, trtllm builds are not getting triggered.
With this change, I am hoping to trigger the build process when the respective files are changed.