[https://nvbugs/5451296][fix] zmq nonblock bug with retry #7019
Conversation
|
Warning Rate limit exceeded@Superjomn has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds retryable non-blocking send to ZeroMqQueue.put_noblock (new keyword-only params retry, wait_time) with input validation and logging on exhaustion; updates two callers to pass retry values; and moves six llmapi unit tests from the CPP block to the PyTorch block in the integration test list. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Proxy / MPI Session
participant Q as ZeroMqQueue
Caller->>Q: put_noblock(obj, retry=N, wait_time=dt)
loop up to N attempts
Q->>Q: try non-blocking send
alt send succeeds
Q-->>Caller: return
else zmq.Again
Q->>Q: sleep(wait_time)
else other error
Q-->>Caller: raise
end
end
Q-->>Caller: log error if exhausted
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
Superjomn
requested review from
Shixiaowei02,
SimengLiu-nv,
Tabrizian,
brb-nv,
hypdeb,
kaiyux,
mikeiovine,
nv-guomingz,
symphonylyh and
zeroepoch
Superjomn
requested review from
litaotju
and removed request for
brb-nv and
kaiyux
|
/bot run |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tensorrt_llm/executor/ipc.py (1)
128-142: Docstring needs style fix (Ruff D205) and clarity on error semantics
- Add a blank line after the summary per D205.
- Clarify that only zmq.Again (EAGAIN) is retried/suppressed; other exceptions propagate.
Apply this diff to update the docstring:
- ''' - Put an object into the queue without blocking, and retry if the send fails. - NOTE: It won't raise any error if the send fails. - - Parameters: - obj (Any): The object to send. - retry (int): The number of times to retry sending the object. - wait_time (float): The time to wait before retrying. - ''' + ''' + Put an object into the queue without blocking, retrying on EAGAIN. + + Only zmq.Again (EAGAIN) is retried and suppressed here; other exceptions are propagated. + + Args: + obj (Any): The object to send. + retry (int): Number of retry attempts on EAGAIN (0 means single try). Must be between 0 and 2000. + wait_time (float): Sleep time in seconds between retries. + '''tensorrt_llm/llmapi/mpi_session.py (1)
438-444: Non-blocking send try/except is now redundant for EAGAINWith the updated put_noblock that internally retries and swallows EAGAIN (logging on exhaustion), this surrounding try/except won’t see EAGAIN anymore. Keeping it is harmless, but you could remove it or narrow it to non-EAGAIN errors if desired.
Optionally simplify to:
self.queue.put_noblock(self.results, retry=2)If you want to preserve handling of non-EAGAIN ZMQError, you can catch and re-raise only when e.errno != zmq.EAGAIN, though put_noblock already re-raises non-EAGAIN in the suggested refactor.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
tensorrt_llm/executor/ipc.py(1 hunks)tensorrt_llm/executor/proxy.py(1 hunks)tensorrt_llm/llmapi/mpi_session.py(1 hunks)tests/integration/test_lists/test-db/l0_a10.yml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
**/*.py: Python code must target Python 3.8+
Python indentation: 4 spaces, no tabs
Maintain module namespace in imports (from package.subpackage import foo; then use foo.SomeClass())
Python file names use snake_case
Python class names use PascalCase
Python functions/methods and local variables use snake_case; variables starting with a number get k_ prefix (e.g., k_99th_percentile)
Global variables use G_ prefixed UPPER_SNAKE_CASE (e.g., G_MY_GLOBAL)
Constants use UPPER_SNAKE_CASE in Python
Avoid shadowing variables from outer scopes in Python
Initialize all externally visible members of a Python class in init
Prefer docstrings for interfaces used outside a file; comments for local code
Use Google-style docstrings for classes and functions (Sphinx-parsable)
Document attributes/variables inline with short docstrings
Avoid reflection when simple alternatives exist (e.g., prefer explicit parameters over dict(**locals()))
In try/except, catch the narrowest exceptions possible
For duck-typing with try/except, keep try body minimal and put logic in else
Files:
tensorrt_llm/llmapi/mpi_session.pytensorrt_llm/executor/ipc.pytensorrt_llm/executor/proxy.py
**/*.{cpp,cxx,cc,cu,h,hpp,hxx,hh,cuh,py}
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
Prepend NVIDIA copyright header (current year) to all source files
Files:
tensorrt_llm/llmapi/mpi_session.pytensorrt_llm/executor/ipc.pytensorrt_llm/executor/proxy.py
🧠 Learnings (1)
📚 Learning: 2025-07-28T17:06:08.621Z
Learnt from: moraxu
PR: NVIDIA/TensorRT-LLM#6303
File: tests/integration/test_lists/qa/examples_test_list.txt:494-494
Timestamp: 2025-07-28T17:06:08.621Z
Learning: In TensorRT-LLM testing, it's common to have both CLI flow tests (test_cli_flow.py) and PyTorch API tests (test_llm_api_pytorch.py) for the same model. These serve different purposes: CLI flow tests validate the traditional command-line workflow, while PyTorch API tests validate the newer LLM API backend. Both are legitimate and should coexist.
Applied to files:
tests/integration/test_lists/test-db/l0_a10.yml
🧬 Code Graph Analysis (2)
tensorrt_llm/executor/ipc.py (2)
tensorrt_llm/_utils.py (1)
nvtx_range_debug(864-888)tensorrt_llm/logger.py (1)
error(125-126)
tensorrt_llm/executor/proxy.py (1)
tensorrt_llm/executor/ipc.py (1)
put_noblock(125-152)
🪛 Ruff (0.12.2)
tensorrt_llm/executor/ipc.py
130-137: 1 blank line required between summary line and description
(D205)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit Check
🔇 Additional comments (2)
tests/integration/test_lists/test-db/l0_a10.yml (1)
37-43: Re-categorizing llmapi tests under PyTorch confirmedThe
unittest/llmapi/*tests now appear exactly once under the PyTorch backend and are not present in any CPP blocks. Everything looks correct.tensorrt_llm/executor/proxy.py (1)
352-352: Adding retry for shutdown sentinel is a good resilience improvementUsing retry=4 reduces spurious shutdown failures when the worker queue is temporarily busy.
|
PR_Github #15693 [ run ] triggered by Bot |
Superjomn
force-pushed
the
fix-runtime-size
branch
from
a7411f1 to
d7cafad
Compare
Superjomn
force-pushed
the
fix-runtime-size
branch
from
d7cafad to
6c4b800
Compare
|
/bot run |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tensorrt_llm/executor/ipc.py (1)
150-157: Replace recursive retry with an iterative loop and avoid logging full object contentsRecursion adds stack overhead and re-enters the function (re-pickling/signing on each attempt via the recursive call), and logging the entire object risks huge logs/PII leakage. An iterative loop is simpler, avoids repeated serialization, and logs concise context.
if self.use_hmac_encryption: data = self._sign_data(data) - try: - self.socket.send(data, flags=zmq.NOBLOCK) - except zmq.Again: - if retry > 0: - time.sleep(wait_time) - self.put_noblock(obj, retry=retry - 1, wait_time=wait_time) - else: - logger.error(f"Failed to send object: {obj}") + attempts = retry + 1 # initial try + retries + for i in range(attempts): + try: + self.socket.send(data, flags=zmq.NOBLOCK) + break + except zmq.Again: + if i < attempts - 1: + time.sleep(wait_time) + continue + logger.error( + f"put_noblock failed after {attempts} attempts " + f"(queue={self.name}, socket={self.socket_type_str.get(self.socket_type, self.socket_type)}, " + f"obj_type={type(obj).__name__}, bytes={len(data)})" + ) + except zmq.ZMQError: + # Propagate non-EAGAIN ZMQ errors to caller + raise
🧹 Nitpick comments (3)
tensorrt_llm/executor/ipc.py (3)
133-141: Docstring: fix D205, switch to Google style, and clarify which errors are suppressed
- Ruff D205: add a blank line after the one-line summary.
- Repo guideline: use Google-style docstrings (“Args”, “Returns”).
- Clarify behavior: only EAGAIN (would-block) is suppressed; pickling or other ZMQ errors can still bubble up.
Apply:
- ''' - Put an object into the queue without blocking, and retry if the send fails. - NOTE: It won't raise any error if the send fails. - - Parameters: - obj (Any): The object to send. - retry (int): The number of times to retry sending the object. - wait_time (float): The time to wait before retrying. - ''' + ''' + Put an object into the queue without blocking. + + Retries on EAGAIN (would block) up to `retry` times with `wait_time` seconds between attempts. + Only EAGAIN is suppressed; serialization errors or other ZMQ errors may propagate. + + Args: + obj (Any): The object to send. + retry (int): Number of retry attempts after the initial try. Must be between 0 and 10. + wait_time (float): Seconds to wait before each retry. + + Returns: + None + '''
143-143: Avoid assert for argument validation; raise ValueError insteadasserts can be stripped with -O and won’t validate in production. Prefer explicit checks with exceptions.
- assert retry >= 0 and retry <= 10, "Retry must be between 0 and 10, adjust the wait_time if needed" + if not (0 <= retry <= 10): + raise ValueError("retry must be between 0 and 10; increase wait_time instead of large retry counts")
1-2: Missing NVIDIA copyright headerPer repo guidelines, prepend the NVIDIA copyright header (current year).
+# Copyright (c) 2025, NVIDIA CORPORATION. All rights reserved.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tensorrt_llm/executor/ipc.py(1 hunks)tests/integration/test_lists/test-db/l0_a10.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/test_lists/test-db/l0_a10.yml
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
**/*.py: Python code must target Python 3.8+
Python indentation: 4 spaces, no tabs
Maintain module namespace in imports (from package.subpackage import foo; then use foo.SomeClass())
Python file names use snake_case
Python class names use PascalCase
Python functions/methods and local variables use snake_case; variables starting with a number get k_ prefix (e.g., k_99th_percentile)
Global variables use G_ prefixed UPPER_SNAKE_CASE (e.g., G_MY_GLOBAL)
Constants use UPPER_SNAKE_CASE in Python
Avoid shadowing variables from outer scopes in Python
Initialize all externally visible members of a Python class in init
Prefer docstrings for interfaces used outside a file; comments for local code
Use Google-style docstrings for classes and functions (Sphinx-parsable)
Document attributes/variables inline with short docstrings
Avoid reflection when simple alternatives exist (e.g., prefer explicit parameters over dict(**locals()))
In try/except, catch the narrowest exceptions possible
For duck-typing with try/except, keep try body minimal and put logic in else
Files:
tensorrt_llm/executor/ipc.py
**/*.{cpp,cxx,cc,cu,h,hpp,hxx,hh,cuh,py}
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
Prepend NVIDIA copyright header (current year) to all source files
Files:
tensorrt_llm/executor/ipc.py
🧬 Code Graph Analysis (1)
tensorrt_llm/executor/ipc.py (2)
tensorrt_llm/_utils.py (1)
nvtx_range_debug(864-888)tensorrt_llm/logger.py (1)
error(125-126)
🪛 Ruff (0.12.2)
tensorrt_llm/executor/ipc.py
130-137: 1 blank line required between summary line and description
(D205)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit Check
🔇 Additional comments (1)
tensorrt_llm/executor/ipc.py (1)
128-133: Good API: keyword-only retry/wait_time with sensible defaultsMaking retry semantics keyword-only avoids accidental arg shifting and makes call sites clearer. Defaults look reasonable.
|
PR_Github #15695 [ run ] triggered by Bot |
|
PR_Github #15693 [ run ] completed with state |
|
PR_Github #15695 [ run ] completed with state |
Signed-off-by: Superjomn <[email protected]>
Signed-off-by: Superjomn <[email protected]>
Superjomn
force-pushed
the
fix-runtime-size
branch
from
6c4b800 to
970287a
Compare
|
/bot run |
|
PR_Github #15793 [ run ] triggered by Bot |
|
PR_Github #15793 [ run ] completed with state |
Signed-off-by: Superjomn <[email protected]>
Signed-off-by: Superjomn <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
Signed-off-by: Superjomn <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
Signed-off-by: Superjomn <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
Signed-off-by: Superjomn <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
Signed-off-by: Superjomn <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
3 participants
Summary by CodeRabbit
Bug Fixes
Documentation
Tests
Description
Fix the zmq send_nonblock issue with retry.
Move llmapi-related unittests to pre-merge to avoid being broken frequently in post-merge.
Test Coverage
GitHub Bot Help
/bot [-h] ['run', 'kill', 'skip', 'reuse-pipeline'] ...Provide a user friendly way for developers to interact with a Jenkins server.
Run
/bot [-h|--help]to print this help message.See details below for each supported subcommand.
Details
run [--reuse-test (optional)pipeline-id --disable-fail-fast --skip-test --stage-list "A10-PyTorch-1, xxx" --gpu-type "A30, H100_PCIe" --test-backend "pytorch, cpp" --add-multi-gpu-test --only-multi-gpu-test --disable-multi-gpu-test --post-merge --extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx" --detailed-log --debug(experimental)]Launch build/test pipelines. All previously running jobs will be killed.
--reuse-test (optional)pipeline-id(OPTIONAL) : Allow the new pipeline to reuse build artifacts and skip successful test stages from a specified pipeline or the last pipeline if no pipeline-id is indicated. If the Git commit ID has changed, this option will be always ignored. The DEFAULT behavior of the bot is to reuse build artifacts and successful test results from the last pipeline.--disable-reuse-test(OPTIONAL) : Explicitly prevent the pipeline from reusing build artifacts and skipping successful test stages from a previous pipeline. Ensure that all builds and tests are run regardless of previous successes.--disable-fail-fast(OPTIONAL) : Disable fail fast on build/tests/infra failures.--skip-test(OPTIONAL) : Skip all test stages, but still run build stages, package stages and sanity check stages. Note: Does NOT update GitHub check status.--stage-list "A10-PyTorch-1, xxx"(OPTIONAL) : Only run the specified test stages. Examples: "A10-PyTorch-1, xxx". Note: Does NOT update GitHub check status.--gpu-type "A30, H100_PCIe"(OPTIONAL) : Only run the test stages on the specified GPU types. Examples: "A30, H100_PCIe". Note: Does NOT update GitHub check status.--test-backend "pytorch, cpp"(OPTIONAL) : Skip test stages which don't match the specified backends. Only support [pytorch, cpp, tensorrt, triton]. Examples: "pytorch, cpp" (does not run test stages with tensorrt or triton backend). Note: Does NOT update GitHub pipeline status.--only-multi-gpu-test(OPTIONAL) : Only run the multi-GPU tests. Note: Does NOT update GitHub check status.--disable-multi-gpu-test(OPTIONAL) : Disable the multi-GPU tests. Note: Does NOT update GitHub check status.--add-multi-gpu-test(OPTIONAL) : Force run the multi-GPU tests in addition to running L0 pre-merge pipeline.--post-merge(OPTIONAL) : Run the L0 post-merge pipeline instead of the ordinary L0 pre-merge pipeline.--extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx"(OPTIONAL) : Run the ordinary L0 pre-merge pipeline and specified test stages. Examples: --extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx".--detailed-log(OPTIONAL) : Enable flushing out all logs to the Jenkins console. This will significantly increase the log volume and may slow down the job.--debug(OPTIONAL) : Experimental feature. Enable access to the CI container for debugging purpose. Note: Specify exactly one stage in thestage-listparameter to access the appropriate container environment. Note: Does NOT update GitHub check status.For guidance on mapping tests to stage names, see
docs/source/reference/ci-overview.mdand the
scripts/test_to_stage_mapping.pyhelper.kill
killKill all running builds associated with pull request.
skip
skip --comment COMMENTSkip testing for latest commit on pull request.
--comment "Reason for skipping build/test"is required. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.reuse-pipeline
reuse-pipelineReuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.