Add validated data args in default factory #1475

nix010 · 2024-10-07T04:36:05Z

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

nix010 · 2024-10-07T04:38:23Z

src/validators/with_default.rs

+                let co_vars = default_factory.getattr(py, "__code__")?.getattr(py, "co_varnames")?;
+                let default_factory_args: &Bound<PyTuple> = co_vars.downcast_bound::<PyTuple>(py)?;


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

codecov · 2024-10-07T04:39:24Z

Codecov Report

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

Project coverage is 89.08%. Comparing base (ab503cb) to head (1f7c844).
Report is 206 commits behind head on main.

Files with missing lines	Patch %	Lines
src/validators/with_default.rs	95.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1475      +/-   ##
==========================================
- Coverage   90.21%   89.08%   -1.13%     
==========================================
  Files         106      112       +6     
  Lines       16339    17849    +1510     
  Branches       36       40       +4     
==========================================
+ Hits        14740    15901    +1161     
- Misses       1592     1928     +336     
- Partials        7       20      +13

Files with missing lines	Coverage Δ
python/pydantic_core/core_schema.py	`94.84% <100.00%> (+0.07%)`	⬆️
src/serializers/type_serializers/with_default.rs	`79.06% <100.00%> (ø)`
src/validators/with_default.rs	`98.44% <95.00%> (-0.63%)`	⬇️

... 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 92a259e...1f7c844. Read the comment docs.

codspeed-hq · 2024-10-07T04:42:54Z

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 · 2024-10-07T12:31:32Z

please review

sydney-runkle

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.

sydney-runkle · 2024-10-09T15:37:52Z

src/validators/with_default.rs

+            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'>";


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

See

pydantic-core/src/input/return_enums.rs

Lines 313 to 327 in 903a1a9

// 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.

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

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 · 2024-10-09T15:59:04Z

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 · 2024-10-09T16:58:51Z

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 · 2024-10-09T17:51:24Z

@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 · 2024-10-09T18:14:29Z

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 · 2024-10-09T18:29:05Z

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

sydney-runkle · 2024-10-13T13:41:02Z

python/pydantic_core/core_schema.py

@@ -2418,10 +2422,14 @@ def with_default_schema(
        metadata: Any other information you want to include with the schema, not used by pydantic-core
        serialization: Custom serialization schema
    """
+
+    has_arg = isfunction(default_factory) and len(signature(default_factory).parameters) > 0


@nix010, this check should reside in pydantic, like we do for the validators and determining their info status 👍

@sydney-runkle is this func is what you've mentioned ?
https://github.com/pydantic/pydantic/blob/60f704721a096dced3b19dd465a1e31495fcd802/pydantic/json_schema.py#L1076.
I don't see where it got called in the process.
Also do you think that this line should be removed in pydantic-core ?

Yep, this line should be removed in pydantic-core, bc the inspect logic should reside in pydantic.

Re inspection, I'd refer to this as an example: https://github.com/pydantic/pydantic/blob/6c3d3b3bb255422fc64eec8e9014016743207ff0/pydantic/_internal/_decorators.py#L514-L548

@sydney-runkle sorry, I took a while but can't find a good place to put this check in the pydantic and accomodate with this, so feel free to take over this one.

@nix010 no worries, I'll take it over today

Viicos · 2024-10-23T14:55:31Z

Replaced by #1491. Thanks @nix010, you'll be added as a co-author on the pydantic PR.

Add validated data args in default factory

9d83ce6

nix010 commented Oct 7, 2024

View reviewed changes

nix010 added 6 commits October 7, 2024 12:15

fix error message depend on python version

72fe9fb

fix error message depend on python version

d4bf43c

fix error message depend on python version

3245cc7

fix error message depend on python version

8be0096

Fix test fail in pypy and python<3.10

0e0dbc3

Fix test fail in pypy and python<3.10

5e5c249

pydantic-hooky bot added the ready for review label Oct 7, 2024

pydantic-hooky bot assigned davidhewitt Oct 7, 2024

Merge branch 'main' into add_validated_data_arg_to_default_factory

94c1c6a

sydney-runkle reviewed Oct 9, 2024

View reviewed changes

Merge branch 'main' into add_validated_data_arg_to_default_factory

ecbc7a9

nix010 added 3 commits October 12, 2024 18:20

rework using bool flag for passing arg

08fcdac

Add comments

8dd5ec2

Merge branch 'main' into add_validated_data_arg_to_default_factory

ed025e1

sydney-runkle reviewed Oct 13, 2024

View reviewed changes

Merge branch 'main' into add_validated_data_arg_to_default_factory

1f7c844

Viicos closed this Oct 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add validated data args in default factory #1475

Add validated data args in default factory #1475

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		let co_vars = default_factory.getattr(py, "__code__")?.getattr(py, "co_varnames")?;
		let default_factory_args: &Bound<PyTuple> = co_vars.downcast_bound::<PyTuple>(py)?;

	// 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)));
	}
	}

Add validated data args in default factory #1475

Add validated data args in default factory #1475

Uh oh!

Conversation

Uh oh!

Change Summary

Related issue number

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Codecov Report

Uh oh!

Uh oh!

CodSpeed Performance Report

Merging #1475 will not alter performance

Summary

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!