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

Add validated data args in default factory #1475

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

nix010
Copy link
Contributor

@nix010 nix010 commented Oct 7, 2024

Change Summary

Add feature pydantic pydantic/pydantic#9789

Related issue number

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

Selected Reviewer: @davidhewitt

Comment on lines 49 to 50
let co_vars = default_factory.getattr(py, "__code__")?.getattr(py, "co_varnames")?;
let default_factory_args: &Bound<PyTuple> = co_vars.downcast_bound::<PyTuple>(py)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open to suggestions on better way to do args checking on the python default factory!

Copy link

codecov bot commented Oct 7, 2024

Codecov Report

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

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

Files with missing lines Patch % Lines
src/validators/with_default.rs 93.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1475      +/-   ##
==========================================
- Coverage   90.21%   89.21%   -1.00%     
==========================================
  Files         106      112       +6     
  Lines       16339    17829    +1490     
  Branches       36       41       +5     
==========================================
+ Hits        14740    15906    +1166     
- Misses       1592     1903     +311     
- Partials        7       20      +13     
Files with missing lines Coverage Δ
src/serializers/type_serializers/with_default.rs 79.06% <100.00%> (ø)
src/validators/with_default.rs 98.38% <93.33%> (-0.69%) ⬇️

... and 48 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 903a1a9...ecbc7a9. Read the comment docs.

Copy link

codspeed-hq bot commented Oct 7, 2024

CodSpeed Performance Report

Merging #1475 will not alter performance

Comparing nix010:add_validated_data_arg_to_default_factory (ecbc7a9) with main (903a1a9)

Summary

✅ 155 untouched benchmarks

@nix010
Copy link
Contributor Author

nix010 commented Oct 7, 2024

please review

Copy link
Member

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

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

This implementation is more simple than I expected thus far - nice work.

Curious to hear what @Viicos thinks given that he helped with the design choices for this API.

Self::DefaultFactory(ref default_factory) => {
// MASSIVE HACK! PyFunction doesn't exist for PyPy,
// ref from https://github.com/pydantic/pydantic-core/pull/161#discussion_r917257635
let is_func = default_factory.getattr(py, "__class__")?.to_string() == "<class 'function'>";
Copy link
Member

Choose a reason for hiding this comment

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

Should we add both branches here so that we can hopefully remove the hack in the long run?

See

// the PyFunction::is_type_of(attr) catches `staticmethod`, but also any other function,
// I think that's better than including static methods in the yielded attributes,
// if someone really wants fields, they can use an explicit field, or a function to modify input
#[cfg(not(PyPy))]
if !is_bound && !attr.is_instance_of::<PyFunction>() {
return Some(Ok((name, attr)));
}
// MASSIVE HACK! PyFunction doesn't exist for PyPy,
// is_instance_of::<PyFunction> crashes with a null pointer, hence this hack, see
// https://github.com/pydantic/pydantic-core/pull/161#discussion_r917257635
#[cfg(PyPy)]
if !is_bound && attr.get_type().to_string() != "<class 'function'>" {
return Some(Ok((name, attr)));
}
}
for an example.

Copy link
Member

Choose a reason for hiding this comment

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

Do we even need the function check here? I assume if we got to this point we know this is a callable bc we've matched on DefaultFactory...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because there's a case ex: default_factory=str in python then the default_factory here will not have __code__ and raise error.

@Viicos
Copy link
Member

Viicos commented Oct 9, 2024

Thanks @nix010 for the implementation.

The proper way (and this will simplify things even more here) to do this is to inspect the default_factory callable in Pydantic, using inspect. The core schema structure will have to be changed to add a new has_values_arg boolean entry (name TBD).

As an existing example, we do this in pydantic._internal.decorators.inspect_validator to determine if validators take the info argument or not.

@nix010
Copy link
Contributor Author

nix010 commented Oct 9, 2024

Thanks @nix010 for the implementation.

The proper way (and this will simplify things even more here) to do this is to inspect the default_factory callable in Pydantic, using inspect. The core schema structure will have to be changed to add a new has_values_arg boolean entry (name TBD).

As an existing example, we do this in pydantic._internal.decorators.inspect_validator to determine if validators take the info argument or not.

@Viicos So my intention is exactly the same. But to inspect the function within fn default_value(), I went with the __code__ attr there. Can you advise me on a better way to do that?

@sydney-runkle
Copy link
Member

@Viicos is referring to https://github.com/pydantic/pydantic/blob/6c3d3b3bb255422fc64eec8e9014016743207ff0/pydantic/_internal/_decorators.py#L514-L548

Basically, we want to inspect the function in pydantic with the inspect module, then modify the core schema accordingly (we'll have to add an argument). We can match on that argument to decide on whether we need to pass the validated data, in pydantic-core.

@nix010
Copy link
Contributor Author

nix010 commented Oct 9, 2024

So my intention is exactly the same. But to inspect the function within fn default_value(), I went with the __code__ attr there. Can you advise me on a better way to do that?

Ok. So there will be a bool var in the model core schema to flag if the data need to pass in to the default_factory or not. Right ?

@Viicos
Copy link
Member

Viicos commented Oct 9, 2024

Ok. So there will be a bool var in the model core schema to flag if the data need to pass in to the default_factory or not. Right ?

Correcet, probably on the field core schema

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

Successfully merging this pull request may close these issues.

4 participants