Support kwargs with from_attributes

[jcsho]

Draft PR based on the #223 details but got stuck on the actual usage in the python api side.

Would like some help defining the expected test cases and if the optional args in the macro_rules changes are expected?

[samuelcolvin]

As per the issue,

I guess the input validator needs to support a tuple of (object, kwargs) too.

Basically validate_typed_dict on the Input implementation for python should also accept a tuple of (object, kwargs).

Does that make sense?

[jcsho]

hi @samuelcolvin , maybe I should've clarified the impl before starting.

I took some time to dig through the code base but still quite confused about the translation from pydantic to pyo3 to rust

So based on my understanding - this is used by pydantic in Model.from_orm(expected_fields, **additional_kwargs)

by adding (object, kwargs) to validate_typed_dict do you mean I should modify the input_abstract and input_python signature to something like

fn validate_typed_dict(
    &'a self,
     strict: bool,
    from_attributes: bool,
+  additional_kwargs: Option<(&'a PyAny, Option<&'a PyDict>)>,
) -> ValResult<GenericMapping<'a>> {
    if from_attributes {    //  <-- does from_attributes relate to this additional_kwargs?
            if let Some((object, kwargs)) = additional_kwargs {
                return Ok(GenericMapping::PyGetAttr(object, kwargs));
            }
   ...
}

and if that's correct, how would I get this additional_kwargs from the typed_dict validate function? does the input parameter get casted into this?

[samuelcolvin]

Not your fault, I think the problem is me 1) not thinking it through enough before creating the issue 2) not giving a clear description of what I have thought of.

Part of this is an in unavoidable result of "thinking by coding" - I often don't know how to do things until I'm half way through doing things.

---

"from orm" will change a lot in V2, not just being renamed to from_attributes:

• if from_attributes=True, passing an object to parse_obj (now called model_validate) it should "just work" - the model is constructed by extracting attributes from the object, instead of extract items from a dict; this should already work
• Similarly if from_attributes=True, a submodel could be populated from an object as it would be from a dict
• there should be no need for an explicit from_orm method

The way to allow a higher priority dict (aka kwargs) together with an object, is to support parsing (my_object, kwargs) as well as just kwargs.

So the signature of validate_typed_dict doesn't need to change, instead we change the logic at

pydantic-core/src/input/input_python.rs

Lines 345 to 346 in da0a6c7

	if from_attributes_applicable(self) {
	Ok(self.into())

to something like

            if from_attributes_applicable(self) {
                Ok(self.into())
            } else if let Ok((obj, kwargs)) = self.extract::<(&PyAny, &PyDict)>() {
                if from_attributes_applicable(self) {
                    // return a generic mapping with the object and kwargs
                } else {
                    // note the error here gives a hint about from_attributes
                    Err(ValError::new(ErrorKind::DictAttributesType, self))
                }

Then the logic in python will take care of passing (input_value, from_attributes_kwargs) to the validate instead of input_value, if from_attributes_kwargs is provided.

Hope that makes sense?

[jcsho]

thanks for the explanation @samuelcolvin!

that cleared it up for sure, I think I implemented most of it through your guidance but still learning to debug this locally - would the test case added to typed-dict here be an example of how the python api uses this new feature?

so setting from_attributes=True and submitting an input with extra attributes than defined in the fields from SchemaValidator? if so I will have to debug some more why it's not working

[samuelcolvin]

Sorry for the slow reply, somehow I missed this.

Yes that looks right.

Let me know when this is ready to review.

[samuelcolvin]

Hi @jcsho when do you think you'll be able to finish this? Let me know if you have any questions.

[jcsho]

@samuelcolvin sorry also didn't see this notification - just got back from vacation so will try to continue tackling this this weekend.

[jcsho]

@samuelcolvin actually I took some time to debug the issue tonight and I found that I might still be confused about the usage. I pushed my current working code to this branch.

1. in validate_typed_dict I attempted to debug by swapping the if-statements to match the expected behaviour discussed above but the code path never reaches into the if-statement here when I run the sample test case here

• what I expected to happen
  • a, b, c fields are extracted as it currently does, but the extra field d should've been extracted to the kwargs field
• what actually happens
  • a, b, c fields are correctly extracted, but any extra fields are ignored
    then I realized maybe it'll always match the current predicate first...

# validate_typed_dict()
if let Ok(dict) = self.cast_as::<PyDict>() {   # always matches for any dict
      return Ok(dict.into());
}
... strict checking

if let Ok((obj, kwargs)) = self.extract::<(&PyAny, &PyDict)>() {  # doesn't reach here
    if from_attributes_applicable(self) {
            return Ok(GenericMapping::PyGetAttr(obj, Some(kwargs)));
    } else {
            return Err(ValError::new(ErrorType::DictAttributesType, self));
    }
}

so maybe I didn't quite understand how the current input can be parsed?

2. debugging - wondering if it's possible to hook into the vscode debugger or any step through debugging for the Rust side?

[samuelcolvin]

as described below, I think the reason your println isn't being called is that the input is wrong.

As for 2), I have no idea, I don't use debug breakpoints, I generally use the dbg! macro.

[samuelcolvin]

your input data passed is still a dict, not a tuple.

I think you need to pass something like

    class Foobar:
        def __init__(self):
            self.a = 1
            self.b = 2
            self.c = 3

test_input = (Foobar(), dict(c=3)

[jcsho]

updated test cases thanks!

[codspeed-hq]

CodSpeed Performance Report

Merging #306 support-kwargs-with-from-attributes (7a71792) will not alter performances.

Summary

🔥 0 improvements
❌ 0 regressions
✅ 80 untouched benchmarks

🆕 0 new benchmarks
⁉️ 0 dropped benchmarks

[codecov-commenter]

Codecov Report

Merging #306 (7a71792) into main (459e59e) will decrease coverage by 0.01%.
The diff coverage is 83.33%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #306      +/-   ##
==========================================
- Coverage   96.51%   96.50%   -0.01%     
==========================================
  Files          87       87              
  Lines        9447     9455       +8     
  Branches        4        4              
==========================================
+ Hits         9118     9125       +7     
- Misses        329      330       +1     

Impacted Files	Coverage Δ
src/validators/dict.rs	95.04% <0.00%> (ø)
src/input/input_python.rs	98.17% <75.00%> (-0.25%)	⬇️
src/validators/union.rs	98.21% <75.00%> (ø)
src/input/return_enums.rs	97.87% <100.00%> (ø)
src/lookup_key.rs	92.38% <100.00%> (+0.15%)	⬆️
src/validators/typed_dict.rs	98.95% <100.00%> (ø)

---

Continue to review full report at Codecov.

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

[jcsho]

Hi @samuelcolvin, took awhile but finally understood what the expected behaviour was. I added the corresponding test cases to the existing typed_dict.from_attributes test.

Just need a review on the Rust side - if the macros re-arrangements of the arguments to allow optional fields is an okay solution or any edge cases you want me to test

[jcsho]

bump @samuelcolvin in case you didn't get to see this is ready for review :)

[samuelcolvin]

This is awesome, thank you so much @jcsho, I'm sorry I didn't see you had completed this until now - entirely my mistake 🙏.