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

skip recursion checks for non-recursive definitions #989

Closed
wants to merge 1 commit into from

Conversation

davidhewitt
Copy link
Contributor

Change Summary

Detects recursion at the pydantic-core level, so that pydantic doesn't have to, enabling low-level performance wins without needing to inline core schemas.

Related issue number

Related to pydantic/pydantic#7611

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

state.recursion_guard.decr_depth();
output
let definition = state.definitions.get(self.validator_id).unwrap();
if definition.recursive {
Copy link
Contributor Author

@davidhewitt davidhewitt Sep 25, 2023

Choose a reason for hiding this comment

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

If I comment out this block I get a segfault in the test suite which gives me confidence the recursion detection is working correctly.

Comment on lines 90 to 95
let value_id = extra.rec_guard.add(value, self.serializer_id).map_err(py_err_se_err)?;
let comb_serializer = extra.definitions.get(self.serializer_id).unwrap();
let r = comb_serializer.serde_serialize(value, serializer, include, exclude, extra);
let definition = extra.definitions.get(self.serializer_id).unwrap();
let r = definition
.value
.serde_serialize(value, serializer, include, exclude, extra);
extra.rec_guard.pop(value_id, self.serializer_id);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if we can / should skip recursion on serializing too?

Comment on lines 71 to 74
let value_id = extra.rec_guard.add(value, self.serializer_id)?;
let comb_serializer = extra.definitions.get(self.serializer_id).unwrap();
let r = comb_serializer.to_python(value, include, exclude, extra);
let definition = extra.definitions.get(self.serializer_id).unwrap();
let r = definition.value.to_python(value, include, exclude, extra);
extra.rec_guard.pop(value_id, self.serializer_id);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar question about serializer recursion.

@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Merging #989 (aeaac56) into main (e610984) will decrease coverage by 0.01%.
The diff coverage is 93.75%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #989      +/-   ##
==========================================
- Coverage   92.99%   92.98%   -0.01%     
==========================================
  Files         106      106              
  Lines       15844    15881      +37     
  Branches       35       35              
==========================================
+ Hits        14734    14767      +33     
- Misses       1103     1107       +4     
  Partials        7        7              
Files Coverage Δ
src/definitions.rs 97.59% <100.00%> (+1.22%) ⬆️
src/serializers/extra.rs 99.10% <ø> (ø)
src/serializers/mod.rs 98.49% <ø> (ø)
src/serializers/type_serializers/definitions.rs 93.93% <100.00%> (+0.18%) ⬆️
src/validators/generator.rs 90.94% <ø> (ø)
src/validators/mod.rs 95.00% <100.00%> (ø)
src/validators/definitions.rs 86.27% <82.85%> (-3.20%) ⬇️

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 e610984...aeaac56. Read the comment docs.

@codspeed-hq
Copy link

codspeed-hq bot commented Sep 25, 2023

CodSpeed Performance Report

Merging #989 will improve performances by 52.98%

Comparing dh/detect-recursive-refs (aeaac56) with main (e610984)

Summary

⚡ 1 improvements
✅ 137 untouched benchmarks

Benchmarks breakdown

Benchmark main dh/detect-recursive-refs Change
test_definition_out_of_tree 7.2 ms 4.7 ms +52.98%

@adriangb
Copy link
Member

This unfortunately doesn't work:

from typing import Optional

from pydantic import BaseModel
from pydantic_core import CoreSchema, SchemaValidator


class ModelA(BaseModel):
    b: 'Optional[ModelB]'


class ModelB(BaseModel):
    a: Optional[ModelA]


cyclic_data = {}
cyclic_data['a'] = {'b': cyclic_data}

schema: CoreSchema = {
    'type': 'definitions',
    'schema': {
        'type': 'definition-ref',
        'schema_ref': '__main__.ModelB:5168279104',
    },
    'definitions': [
        {
            'type': 'model',
            'cls': ModelA,
            'schema': {
                'type': 'model-fields',
                'fields': {
                    'b': {
                        'type': 'model-field',
                        'schema': {
                            'type': 'nullable',
                            'schema': {
                                'type': 'definition-ref',
                                'schema_ref': '__main__.ModelB:5168279104'
                            }
                        }
                    }
                },
                'model_name': 'ModelA'
            },
            'ref': '__main__.ModelA:5168251248'
        },
        {
            'type': 'model',
            'cls': ModelB,
            'schema': {
                'type': 'model-fields',
                'fields': {
                    'a': {
                        'type': 'model-field',
                        'schema': {
                            'type': 'nullable',
                            'schema': {
                                'type': 'definition-ref',
                                'schema_ref': '__main__.ModelA:5168251248'
                            }
                        }
                    }
                },
                'model_name': 'ModelB'
            },
            'ref': '__main__.ModelB:5168279104'
        }
    ]
}

validator = SchemaValidator(schema)
validator.validate_python(cyclic_data) # segfault

@davidhewitt
Copy link
Contributor Author

Right yes that's a more complex case; I think we could analyze all the dependencies of definitions in a similar fashion to how this detects recursion, to detect cycles. Will probably follow up next week.

@adriangb
Copy link
Member

That's already what we're doing in Python. If we have to do that here there's not much of a win.

@davidhewitt
Copy link
Contributor Author

Closing in favour of #992 and #993 as a route to avoid using defs for many simple cases.

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.

2 participants