test(deploy): add SGLang JSON logging coverage - #12085
Conversation
Signed-off-by: Kavita Narang <knarang@nvidia.com>
|
/ok to test a866864 |
WalkthroughThe SGLang deployment workflows now run the ChangesSGLang logging validation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
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 `@tests/deploy/test_deploy.py`:
- Around line 264-276: Update the agg_logging validation around
frontend_pod.logs and find_structured_json_log so it captures the frontend log
baseline before the inference request, then searches only log output emitted
afterward. Keep the existing structured-record assertion and logger details, but
ensure startup or health-check records cannot satisfy the inference-time logging
check.
- Around line 190-195: Replace the assert in the agg_logging branch of the
deployment test with an explicit pytest.fail(...) or raised exception when
logging_config["jsonl_enabled"] is false. Also update the related structured-log
check below to use explicit failure handling, ensuring both runtime validations
execute under python -O.
🪄 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: CHILL
Plan: Enterprise
Run ID: fe02a3a5-6bcb-4e49-965d-7bf321e362cf
📒 Files selected for processing (3)
.github/workflows/post-merge-ci.yml.github/workflows/pr.yamltests/deploy/test_deploy.py
|
Signed-off-by: Kavita Narang <knarang@nvidia.com>
|
/ok to test be5e9f1 |
Signed-off-by: Kavita Narang <knarang@nvidia.com>
Signed-off-by: Kavita Narang <knarang@nvidia.com>
Signed-off-by: Kavita Narang <knarang@nvidia.com>
…gging Signed-off-by: Kavita Narang <knarang@nvidia.com> # Conflicts: # .github/workflows/nightly-ci.yml # examples/backends/sglang/deploy/agg_logging.yaml # examples/backends/sglang/deploy/v1beta1/agg_logging.yaml # tests/deploy/test_dgd.py
|
/ok to test 1f855e5 |
|
|
@ai-dynamo/dynamo-operator-codeowners @ai-dynamo/dynamo-backend-sglang-codeowners — review requested for PR #12085. Current exact head 1f855e5 is conflict-free. All seven required merge checks pass, all five review threads are resolved, and the exact-head sglang Nightly Deploy Test / agg_logging job passed: The aggregate Nightly workflow is red in other lanes, but the PR-specific target job passed. Please review and approve if the change is acceptable. |
Signed-off-by: Kavita Narang <knarang@nvidia.com>
Signed-off-by: Kavita Narang <knarang@nvidia.com>
|
/ok to test 122b028 |
Overview:
Add deployment coverage for SGLang structured JSONL logging through the
agg_loggingprofile.Details:
agg_loggingto the post-merge and scheduled-nightly SGLang deployment matricesimage_taginput required by the shared deploy workflowDYN_LOGGING_JSONLruntime configurationpytest.failsglang-agg-loggingresource name in both API manifest versionsWhere should reviewer start?
tests/deploy/test_dgd.pyfor the deployment test and post-inference validationtests/deploy/dgd_utils.pyfor structured-log validation helperstests/deploy/test_dgd_utils.pyfor focused helper coverage.github/workflows/nightly-ci.ymland.github/workflows/post-merge-ci.ymlRelated Issues
Validation
122b028e55a6c6e439bf4d6f97241f5ebc267842.DCO,copyright-checks,codeowners,pre-merge-status-check,backend-status-check,dynamo-status-check, anddeploy-status-check.release=false,run_tests=true, and profiles["agg","agg_logging"].sglang Nightly Deploy Test / agg_loggingjob 98017495851 passed. Its JUnit artifact reports one test, zero failures, and zero errors fortests.deploy.test_dgd::test_deployment[sglang-agg_logging].DYN_LOGGING_JSONL=1, a successful HTTP 200 inference response, and a newly emitted structured JSON log record containing the requiredtime,level,target, andmessagefields. The validator foundtarget=dynamo_llm::http::service::metricsandmessage=request received.401 Unauthorizedand enteredImagePullBackOff; separately,tests.serve.test_sglang::test_sglang_deployment[disaggregated_router-4]reporteddynamo_component_requests_totalcount 0 instead of at least 6. Neither failing test path is changed by this PR, and neither failure exercises the successfulagg_loggingdeployment test.