fix(llm): propagate deployment concurrency to Pylon - #1947
Open
FamousDirector wants to merge 1 commit into
Open
FamousDirector wants to merge 1 commit into
FamousDirector wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (14)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughChangesThe launch path now carries Request propagation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant NVCF
participant ICMS
participant WorkerQueue
participant Translator
participant Pylon
NVCF->>ICMS: Send maxRequestConcurrency
ICMS->>WorkerQueue: Preserve launch specification value
WorkerQueue->>Translator: Provide MaxRequestConcurrency
Translator->>Pylon: Set --max-engine-concurrency
Merge Risk: ⚪ Minimal · up to The deployment concurrency value reaches regular container and Helm Pylon workers while legacy and live-stat fallbacks remain intact, with no merge-blocking risk established. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Carry maxRequestConcurrency from the NVCF deployment GPU specification through ICMS to the Pylon worker. Prefer it over the legacy environment value. Fixes #1946 Signed-off-by: jcameron <jcameron@nvidia.com>
FamousDirector
force-pushed
the
jcameron/fix-pylon-concurrency-1946
branch
from
September 17, 2026 00:43
3614987 to
707999c
Compare
FamousDirector
changed the title
Contributor
🛡️ CodeQL Analysis🚨 Found 11 issue(s) Severity Breakdown:
📋 Top Issues🔗 View full details in Security tab 🕐 Last updated: 2026-09-17 00:51:25 UTC | Commit: 707999c |
barrygreengus
approved these changes
Sep 17, 2026
Max-NV
approved these changes
Sep 17, 2026
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.
TL;DR
Propagate each NVCF deployment's
maxRequestConcurrencyvalue through ICMS to the worker launch specification. Use that typed value to configure Pylon's static maximum engine concurrency fallback.Additional Details
Pylon needs a capacity limit when the inference engine does not publish live
max_engine_concurrencystatistics. The source of that limit is the selected NVCF deployment GPU specification.This change carries
maxRequestConcurrencyfromGpuSpecificationEntityinto the NVCF-to-ICMS request, the ICMS launch model, and the BYOC worker queue message. The Go workload translator then adds--max-engine-concurrency=<value>to the Pylon sidecar for both container-based and Helm-based LLM functions.The typed deployment value is authoritative when it is present. Reading
MAX_REQUEST_CONCURRENCYremains only as a mixed-version fallback for queue messages created by older services during rollout. Live inference-engine statistics continue to take precedence over Pylon's static command-line fallback.No diagram update is needed because this adds a field to the existing launch path without changing component topology.
Customer Release Notes
LLM request routing now uses each deployment's configured request concurrency as a static worker-capacity fallback when live engine statistics are unavailable.
For the Reviewer
Review the deployment-field propagation across NVCF, ICMS, and the BYOC queue model, then the precedence logic in
pkg/icms-translate/translate/function/llm.go. The Go unit test intentionally provides different typed and legacy environment values to verify that the deployment field wins.For QA
Passed:
IcmsClientTest: 9 tests.CreateInstanceApiModelConverterTest,ByocMessageGeneratorTest, andByocSqsMessageModelTest: 27 tests.GOWORK=off GOFLAGS=-mod=vendor go test ./pkg/icms-translate/... -count=1.bazel test //src/libraries/go/lib/pkg/icms-translate/translate/function:function_test --test_output=errors.git diff --check origin/main...HEAD.The local Go codegen command could not run because
deepcopy-genis not installed. The new Go field is a scalar and does not change generated deepcopy code. Thego-lib codegenCI check passed.QA needed: No.
Issues
Fixes #1946
Dependencies
None. No license or NOTICE changes.
Checklist
Summary by CodeRabbit