Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix serialize_as_any with recursive ref #1359

Closed

Conversation

nix010
Copy link
Contributor

@nix010 nix010 commented Jul 1, 2024

BaseModel.model_dump[_xxx](serialize_as_any = True) breaks with models with recursive attribute types (not values)
pydantic/pydantic#9670

Related issue number

fix pydantic/pydantic#9670

Checklist

  • Unit tests for the changes exist
  • Documentation reflects the changes where applicable
  • Pydantic tests pass with this pydantic-core (except for expected changes)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Copy link

codspeed-hq bot commented Jul 1, 2024

CodSpeed Performance Report

Merging #1359 will not alter performance

Comparing nix010:fix-serialize-as-any-with-recursive-ref (d6fbaab) with main (e3eff5c)

Summary

✅ 155 untouched benchmarks

next: Optional['Node'] = None
value: int = 42

schema = core_schema.definitions_schema(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sydney-runkle I tested the python code and all good but how do I make an equivalent schema for the test here ?

from __future__ import annotations

import traceback

import pydantic


class Model1(pydantic.BaseModel):
    recursive: Model1 | None


class Model2(pydantic.BaseModel):
    recursive: pydantic.SerializeAsAny[Model2] | None

print(Model1(recursive=None).model_dump())
print(Model1(recursive=None).model_dump_json())
print(Model2(recursive=None).model_dump())
print(Model2(recursive=None).model_dump_json())

# All fail:
try:
    print(Model1(recursive=None).model_dump(serialize_as_any=True))
except:
    traceback.print_exc()

try:
    print(Model1(recursive=None).model_dump_json(serialize_as_any=True))
except:
    traceback.print_exc()

try:
    print(Model2(recursive=None).model_dump(serialize_as_any=True))
except:
    traceback.print_exc()

try:
    print(Model2(recursive=None).model_dump_json(serialize_as_any=True))
except:
    traceback.print_exc()

Copy link
Member

Choose a reason for hiding this comment

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

Good question - I normally just do

from pydantic._internal._core_utils import pretty_print_core_schema
from pydantic import BaseModel

class SomeModel(BaseModel): ...

pretty_print_core_schema(SomeModel.__pydantic_core_schema__)

And then go from that representation in order to build up an equivalent core schema

@@ -93,8 +94,12 @@ impl TypeSerializer for DefinitionRefSerializer {
) -> PyResult<PyObject> {
self.definition.read(|comb_serializer| {
let comb_serializer = comb_serializer.unwrap();
let mut guard = extra.recursion_guard(value, self.definition.id())?;
comb_serializer.to_python(value, include, exclude, guard.state())
if extra.duck_typing_ser_mode != DuckTypingSerMode::Inferred {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

so what I found is that the outer serializer call to rescursive serializer but also put the self.definition.id() in the stack already. So when the rescursive serializer start to eval, the root model of it have the same self.definition.id() result in raise duplicated id error.

Checking for DuckTypingSerMode::Inferred is because that what I saw as a flag when the rescursive serializer started. I could be wrong so very open for suggestions !

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, seems like a clever find. I think I'd like to have @davidhewitt take a look at this as well before we merge, he should be back soon :).

Copy link
Contributor

@davidhewitt davidhewitt Sep 26, 2024

Choose a reason for hiding this comment

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

This change (to not introduce recursion guards) is the cause of the segfaults in the tests in CI. (The tests now have unbounded recursion and blow the stack.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have fixed the issue!

@nix010 nix010 changed the title [WIP] fix serialize_as_any with recursive ref Fix serialize_as_any with recursive ref Jul 1, 2024
@sydney-runkle
Copy link
Member

@NeevCohen, I'm finally back, and can review this either this evening or tomorrow morning!

@NeevCohen
Copy link
Contributor

@sydney-runkle Welcome back! I think you meant to tag @nix010 😅

@sydney-runkle
Copy link
Member

@NeevCohen ah yes, oops! Thanks! Meant to tag @nix010 :)

Copy link

codecov bot commented Sep 28, 2024

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.21%. Comparing base (ab503cb) to head (d6fbaab).
Report is 190 commits behind head on main.

Files with missing lines Patch % Lines
src/serializers/type_serializers/definitions.rs 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1359      +/-   ##
==========================================
- Coverage   90.21%   89.21%   -1.00%     
==========================================
  Files         106      112       +6     
  Lines       16339    17818    +1479     
  Branches       36       41       +5     
==========================================
+ Hits        14740    15897    +1157     
- Misses       1592     1901     +309     
- Partials        7       20      +13     
Files with missing lines Coverage Δ
src/serializers/extra.rs 96.45% <100.00%> (-3.08%) ⬇️
src/serializers/type_serializers/definitions.rs 84.52% <90.00%> (-15.48%) ⬇️

... and 49 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3eff5c...d6fbaab. Read the comment docs.

@davidhewitt davidhewitt mentioned this pull request Oct 7, 2024
4 tasks
@sydney-runkle
Copy link
Member

Closing in favor of #1478

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants