[Feature]: Use pydantic validation in observability.py config by cern1710 · Pull Request #26637 · vllm-project/vllm

cern1710 · 2025-10-11T14:12:10Z

Part of #26366

Introduced @field_validator to show_hidden_metrics_for_version to ensure proper version formatting
Introduced @field_validator to otlp_traces_endpoint to ensure it starts with either 'http://' or 'https://'
Added @model_validator to validate dependency between collect_detailed_traces and otlp_traces_endpoint

Purpose

Test Plan

Test Result

Essential Elements of an Effective PR Description Checklist

The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
The test plan, such as providing test command.
The test results, such as pasting the results comparison before and after, or e2e results
(Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
(Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

github-actions · 2025-10-11T14:12:18Z

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors.

You ask your reviewers to trigger select CI tests on top of fastcheck CI.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can either: Add ready label to the PR or enable auto-merge.

If you have any questions, please reach out to us on Slack at https://slack.vllm.ai.

🚀

gemini-code-assist

Code Review

This pull request refactors the ObservabilityConfig to use Pydantic validators, which is a good step towards improving configuration clarity and robustness. However, I've identified a critical issue in the parsing logic for collect_detailed_traces that could lead to silent misconfiguration. Additionally, an important validation check for OpenTelemetry dependencies has been removed, which I consider a regression from the previous fail-fast behavior. My review includes suggestions to address both of these points.

vllm/config/observability.py

chatgpt-codex-connector · 2025-10-11T14:14:45Z

💡 Codex Review

https://github.com/vllm-project/vllm/blob/688edaded61796634ff030702a07b6824cb96ff2/vllm/config/observability.py#L107-L116
Preserve all requested detailed trace modules

The new _parse_collect_traces validator splits only the first list element and discards the rest (value = value[0].split(',')). When the CLI is used with multiple --collect-detailed-traces values such as --collect-detailed-traces model worker, argparse already supplies ['model', 'worker']. After this change the list becomes ['model'], so any modules after the first (e.g. worker) are silently ignored and the corresponding metrics are never collected. The previous __post_init__ split values only when a single comma‑separated string was provided, keeping genuine multi-item lists intact. Consider checking for a comma before splitting or iterating over all list items so every requested module is preserved.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Signed-off-by: Samuel Wu <[email protected]>

Signed-off-by: Sam/Samuel <[email protected]>

vllm/config/observability.py

Signed-off-by: Samuel Wu <[email protected]>

cern1710 · 2025-10-13T17:00:49Z

One note, but I've moved the if not is_otel_available() and self.otlp_traces_endpoint is not None: check inside _validate_tracing_config().

Otherwise, I've applied your suggestions in the most recent commit @hmellor

vllm/config/observability.py

Co-authored-by: Harry Mellor <[email protected]> Signed-off-by: Sam/Samuel <[email protected]>

hmellor

Thanks for the PR, can you:

move _parse_collect_detailed_traces into the if block and make that an after field validator
make the rest of __post_init__ into an after field validator for otlp_traces_endpoint

hmellor · 2025-10-13T17:09:17Z

vllm/config/observability.py

+    @field_validator("collect_detailed_traces", mode="before")
+    @classmethod
+    def _validate_collect_detailed_traces(cls, value: Any) -> list[DetailedTraceModules] | None:
+        if value in (None, "", []):
+            return None
+
+        items: list[str] = []
+
+        def add(obj: Any):
+            if obj is None:
+                return
+            elif isinstance(obj, str):
+                items.extend(part.strip().lower() for part in obj.split(","))
+            elif isinstance(obj, (list, tuple, set)):
+                for x in obj:
+                    add(x)
+            else:
+                items.append(str(obj).strip().lower())
+
+        add(value)
+
+        out: list[str] = []
+        seen: set[str] = set()
+        for item in items:
+            if item and item not in seen:
+                seen.add(item)
+                out.append(item)
+
+        if not out:
+            return None
+        return cast(list[DetailedTraceModules], out)


This still isn't quite right. You can change it to an after validator because the input will always pass pydantic's built in validation. We just need a post processing step.

collect_detailed_traces will always be a list[str] or None. The special case is if the list only has one element, and that element is a comma separated list. This is why we split the 0th element in the original implementation.

Okay, on it!

Signed-off-by: Samuel Wu <[email protected]>

cern1710 · 2025-10-13T18:31:11Z

Thank you for the feedback @hmellor! I've made the following changes:

Separate _validate_otlp_available() after field validator to check is_otel_available
Changed _validate_collect_detailed_traces() as an after validator, and changed how to validate it based on specified edge cases

The only problem seems to be that inputs that look like "model, worker" wouldn't be accepted by the after validator with the list[DetailedTraceModules] type, so I've changed it to list[str] for now. Can you confirm that this is intended, or that I should split the current _validate_collect_detailed_traces() method into separate before and after validators?

Edit: I've reverted collect_detailed_traces as a list[DetailedTraceModules] type.

Signed-off-by: Samuel Wu <[email protected]>

…vllm into pydantic-observability

Signed-off-by: Harry Mellor <[email protected]>

hmellor · 2025-10-15T14:51:19Z

Thank you again for the PR, I've pushed a commit that simplifies the logic of the new validators you wrote.

It's not worth writing a fancy validator for the comma separated string use case because it's legacy and I hope to deprecate some day.

…roject#26637) Signed-off-by: Samuel Wu <[email protected]> Signed-off-by: Sam/Samuel <[email protected]> Signed-off-by: Harry Mellor <[email protected]> Co-authored-by: Harry Mellor <[email protected]> Signed-off-by: Alberto Perdomo <[email protected]>

…roject#26637) Signed-off-by: Samuel Wu <[email protected]> Signed-off-by: Sam/Samuel <[email protected]> Signed-off-by: Harry Mellor <[email protected]> Co-authored-by: Harry Mellor <[email protected]>

…roject#26637) Signed-off-by: Samuel Wu <[email protected]> Signed-off-by: Sam/Samuel <[email protected]> Signed-off-by: Harry Mellor <[email protected]> Co-authored-by: Harry Mellor <[email protected]> Signed-off-by: 0xrushi <[email protected]>

…roject#26637) Signed-off-by: Samuel Wu <[email protected]> Signed-off-by: Sam/Samuel <[email protected]> Signed-off-by: Harry Mellor <[email protected]> Co-authored-by: Harry Mellor <[email protected]>

cern1710 requested review from ProExpertProg, WoosukKwon, hmellor, houseroad, mgoin, robertgshaw2-redhat, simon-mo, tlrmchlsmth, yewentao256 and youkaichao as code owners October 11, 2025 14:12

gemini-code-assist bot reviewed Oct 11, 2025

View reviewed changes

vllm/config/observability.py Outdated Show resolved Hide resolved

vllm/config/observability.py Outdated Show resolved Hide resolved

cern1710 force-pushed the pydantic-observability branch from 688edad to 7c20b90 Compare October 11, 2025 14:14

cern1710 added 3 commits October 11, 2025 22:21

Use pydantic validation

aa1f879

Signed-off-by: Samuel Wu <[email protected]>

Consistent naming of model validator

874ffc7

Signed-off-by: Samuel Wu <[email protected]>

Fix errors from pre-commit

62361cd

Signed-off-by: Samuel Wu <[email protected]>

cern1710 force-pushed the pydantic-observability branch from 1c32e22 to 62361cd Compare October 11, 2025 14:21

cern1710 added 4 commits October 11, 2025 22:28

Line too long error

363b058

Signed-off-by: Samuel Wu <[email protected]>

Fix all pre-commit complaints

bac2fba

Signed-off-by: Samuel Wu <[email protected]>

Implement suggestions from Gemini

e752b2b

Signed-off-by: Samuel Wu <[email protected]>

Add space

fe52cb4

Signed-off-by: Samuel Wu <[email protected]>

hmellor mentioned this pull request Oct 13, 2025

[Feature]: Improve config validation using Pydantic #26366

Closed

14 tasks

cern1710 added 2 commits October 14, 2025 00:05

Merge branch 'main' into pydantic-observability

bcbae0f

Signed-off-by: Sam/Samuel <[email protected]>

Update observability.py

66c1d79

Signed-off-by: Sam/Samuel <[email protected]>

hmellor requested changes Oct 13, 2025

View reviewed changes

vllm/config/observability.py Outdated Show resolved Hide resolved

vllm/config/observability.py Outdated Show resolved Hide resolved

vllm/config/observability.py Outdated Show resolved Hide resolved

cern1710 and others added 2 commits October 14, 2025 00:57

Implement suggested changes

0d09f0b

Signed-off-by: Samuel Wu <[email protected]>

Merge branch 'main' into pydantic-observability

f3900e9

hmellor reviewed Oct 13, 2025

View reviewed changes

vllm/config/observability.py Outdated Show resolved Hide resolved

cern1710 and others added 3 commits October 13, 2025 18:06

Update vllm/config/observability.py

ac5efbc

Co-authored-by: Harry Mellor <[email protected]> Signed-off-by: Sam/Samuel <[email protected]>

Update vllm/config/observability.py

ca786b7

Co-authored-by: Harry Mellor <[email protected]> Signed-off-by: Sam/Samuel <[email protected]>

Merge branch 'main' into pydantic-observability

23b5e94

hmellor reviewed Oct 13, 2025

View reviewed changes

fix pre-commit error

a279fa2

Signed-off-by: Samuel Wu <[email protected]>

cern1710 force-pushed the pydantic-observability branch from 18320bd to a279fa2 Compare October 13, 2025 17:15

cern1710 and others added 2 commits October 14, 2025 02:17

suggested changes

bb70df5

Signed-off-by: Samuel Wu <[email protected]>

Merge branch 'main' into pydantic-observability

fb5b8f5

cern1710 and others added 4 commits October 14, 2025 02:40

revert type changes

589bb00

Signed-off-by: Samuel Wu <[email protected]>

Merge branch 'pydantic-observability' of https://github.com/cern1710/…

c219451

…vllm into pydantic-observability

Simplify new validators

dabad8b

Signed-off-by: Harry Mellor <[email protected]>

Merge branch 'main' into pydantic-observability

9f8f6b1

hmellor approved these changes Oct 15, 2025

View reviewed changes

hmellor enabled auto-merge (squash) October 15, 2025 14:51

github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 15, 2025

hmellor merged commit 14f8456 into vllm-project:main Oct 15, 2025
46 checks passed

Uh oh!

Conversation

cern1710 commented Oct 11, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Test Plan

Test Result

Uh oh!

github-actions bot commented Oct 11, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector bot commented Oct 11, 2025

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cern1710 commented Oct 13, 2025

Uh oh!

Uh oh!

hmellor left a comment

Choose a reason for hiding this comment

Uh oh!

hmellor Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

cern1710 Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

cern1710 commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hmellor commented Oct 15, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cern1710 commented Oct 11, 2025 •

edited by github-actions bot

Loading

cern1710 commented Oct 13, 2025 •

edited

Loading