Skip to content

Fix: Exclude computed fields during pydantic MP serialization#845

Merged
mergify[bot] merged 1 commit into
vllm-project:mainfrom
SkiHatDuckie:fix-encoding-test
Jun 24, 2026
Merged

Fix: Exclude computed fields during pydantic MP serialization#845
mergify[bot] merged 1 commit into
vllm-project:mainfrom
SkiHatDuckie:fix-encoding-test

Conversation

@SkiHatDuckie

@SkiHatDuckie SkiHatDuckie commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Summary

See the linked issue for more details. In short, fields with the@computed_field decorator are included in model_dump (and similar) by default, but are then treated as extra fields when passed to model_validate, leading to duplicates appearing in the final deserialized object.

Details

  • Set exclude_computed_fields=True in Serializer.to_dict_pydantic and .to_sequence_pydantic
  • Remove pytest.mark.xfail from test_encode_decode_generative

Test Plan

tox -e test-unit -- tests/unit/utils/test_encoding.py

Related Issues


  • "I certify that all code in this PR is my own, except as noted below."

Use of AI

  • Includes code generated or substantially modified by an AI agent
  • Includes tests generated or substantially modified by an AI agent

NOTE: the Generated-by or Assisted-by trailers should be used in git commit messages when code or tests were generated or substantially modified by an AI agent, as described in the project's DEVELOPING.md file.


git log

commit 4c59acb
Author: SkiHatDuckie SkiHatDuckie@gmail.com
Date: Wed Jun 24 15:02:04 2026 -0400

Exclude computed fields during pydantic model dump

Signed-off-by: SkiHatDuckie <SkiHatDuckie@gmail.com>

Signed-off-by: SkiHatDuckie SkiHatDuckie@gmail.com

Signed-off-by: SkiHatDuckie <SkiHatDuckie@gmail.com>

@sjmonson sjmonson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mergify

mergify Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Queued — the merge queue status continues in this comment ↓.

@dbutenhof dbutenhof left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose my concern would be if we're ever using this in a context where we want computed fields to be serialized (e.g., for a report) -- presumably that'd never be in a context where we're going to re-deserialize and encounter the problem, but this is one of those places where it's hard to get a full call dependency graph; and if Sam thinks this is safe, I'll throw on my +1 on the basis that, at least in isolation, the change looks OK.

@mergify

mergify Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Merge Queue Status

This pull request spent 28 minutes 23 seconds in the queue, including 28 minutes 4 seconds running CI.

Required conditions to merge
  • any of [🛡 GitHub repository ruleset rule Merge Requirements]:
    • check-success = @github-actions/quality (3.10) / type-checks
    • check-neutral = @github-actions/quality (3.10) / type-checks
    • check-skipped = @github-actions/quality (3.10) / type-checks
  • any of [🛡 GitHub repository ruleset rule Merge Requirements]:
    • check-success = @github-actions/quality (3.10) / precommit-checks
    • check-neutral = @github-actions/quality (3.10) / precommit-checks
    • check-skipped = @github-actions/quality (3.10) / precommit-checks
  • any of [🛡 GitHub repository ruleset rule Merge Requirements]:
    • check-success = @github-actions/quality (3.10) / quality-checks
    • check-neutral = @github-actions/quality (3.10) / quality-checks
    • check-skipped = @github-actions/quality (3.10) / quality-checks
  • any of [🛡 GitHub repository ruleset rule Merge Requirements]:
    • check-success = @github-actions/tests (3.10) / e2e-tests
    • check-neutral = @github-actions/tests (3.10) / e2e-tests
    • check-skipped = @github-actions/tests (3.10) / e2e-tests
  • any of [🛡 GitHub repository ruleset rule Merge Requirements]:
    • check-success = @github-actions/tests (3.10) / integration-tests
    • check-neutral = @github-actions/tests (3.10) / integration-tests
    • check-skipped = @github-actions/tests (3.10) / integration-tests
  • any of [🛡 GitHub repository ruleset rule Merge Requirements]:
    • check-success = @github-actions/tests (3.10) / unit-tests
    • check-neutral = @github-actions/tests (3.10) / unit-tests
    • check-skipped = @github-actions/tests (3.10) / unit-tests
  • any of [🛡 GitHub repository ruleset rule Merge Requirements]:
    • check-success = @github-actions/update-description
    • check-neutral = @github-actions/update-description
    • check-skipped = @github-actions/update-description

mergify Bot added a commit that referenced this pull request Jun 24, 2026
@mergify mergify Bot added the queued label Jun 24, 2026
@sjmonson

Copy link
Copy Markdown
Collaborator

I suppose my concern would be if we're ever using this in a context where we want computed fields to be serialized (e.g., for a report)

Had this thought at first but the title/description is misleading. This only changes the encoding code which is used to pass objects from one process to another.

@sjmonson sjmonson changed the title Fix: Exclude computed fields during pydantic model dump Fix: Exclude computed fields during pydantic MP serialization Jun 24, 2026
@mergify mergify Bot merged commit 009a331 into vllm-project:main Jun 24, 2026
13 checks passed
@mergify mergify Bot removed the queued label Jun 24, 2026
@dbutenhof

Copy link
Copy Markdown
Collaborator

I suppose my concern would be if we're ever using this in a context where we want computed fields to be serialized (e.g., for a report)

Had this thought at first but the title/description is misleading. This only changes the encoding code which is used to pass objects from one process to another.

Yeah, that was the usage I found -- but there was a callback protocol that broke the call chain and I wasn't 100% certain we couldn't get there from somewhere else... though it seemed improbable. I'm reassured that you're comfortable with that diagnosis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

serializer.deserialize(serialized) duplicate the last dict data

3 participants