Remove code/features made redundant by JsonOrPython validator/serializer #599
Conversation
CodSpeed Performance ReportMerging #599 Summary
Benchmarks breakdown
|
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## json-or-python #599 +/- ##
==================================================
+ Coverage 94.07% 94.11% +0.03%
==================================================
Files 98 98
Lines 13260 13178 -82
Branches 25 25
==================================================
- Hits 12475 12402 -73
+ Misses 779 770 -9
Partials 6 6
Continue to review full report in Codecov by Sentry.
|
| def isinstance_json( | ||
| self, | ||
| input: 'str | bytes | bytearray', | ||
| *, | ||
| strict: 'bool | None' = None, | ||
| context: Any = None, | ||
| self_instance: 'Any | None' = None, | ||
| ) -> bool: ... | ||
| def validate_assignment( |
There was a problem hiding this comment.
This isn't being used in pydantic, but I don't see is_instance_python being used anywhere in pydantic proper either. @samuelcolvin is this functionality that we just never ended up using, or am I missing something?
| impl IntoPy<PyObject> for InputType { | ||
| fn into_py(self, py: Python<'_>) -> PyObject { | ||
| match self { | ||
| Self::Json => intern!(py, "json").into(), | ||
| Self::Python => intern!(py, "python").into(), | ||
| } |
There was a problem hiding this comment.
Used by validator functions in the ValidationInfo struct
| fn input_is_instance(&self, class: &PyAny, _json_mask: u8) -> PyResult<bool> { | ||
| // See PyO3/pyo3#2694 - we can't use `is_instance` here since it requires PyType, | ||
| // and some check objects are not types, this logic is lifted from `is_instance` in PyO3 | ||
| let result = unsafe { ffi::PyObject_IsInstance(self.as_ptr(), class.as_ptr()) }; | ||
| py_error_on_minusone(self.py(), result)?; | ||
| Ok(result == 1) | ||
| } |
There was a problem hiding this comment.
The comment doesn't seem to be true, I replaced this with calling the method on &PyAny and it works fine, no tests fail here or in Pydantic. I did notice some alleged benchmark regressions for code that is not touched by any other changes here as far as I can tell, so could this be a performance thing? If so I can re-add it (although personally I would happily give up what seems like a very small performance difference to not use unsafe).
There was a problem hiding this comment.
Ok the issue was simpler and had nothing to do with unsafe: the JsonInput version of this short-circuited, but by removing this I was causing the conversion from json -> python object -> check the type. I now added back the short circuit but via matches!(extra.mode, InputType::Python) which is not only more explicit but I suspect might be faster.
| #[derive(Copy, Clone, Debug)] | ||
| pub enum JsonType { | ||
| Null = 0b10000000, | ||
| Bool = 0b01000000, | ||
| Int = 0b00100000, | ||
| Float = 0b00010000, | ||
| String = 0b00001000, | ||
| Array = 0b00000100, | ||
| Object = 0b00000010, | ||
| } |
There was a problem hiding this comment.
Although we won't need this anymore, I'm really curious why we had C-style tags here. What did that do for us that plain enums didn't? I was kinda confused when I saw json_mask: u8.
src/validators/dataclass.rs
Outdated
| if input.input_is_instance(class, 0)? { | ||
| if input.to_object(py).as_ref(py).is_instance(class)? { |
There was a problem hiding this comment.
See above comment about possible performance advantage of our current unsafe implementation.
| mode: ValidationMode, | ||
| mode: InputType, |
There was a problem hiding this comment.
I realize I just created ValidationMode in #598, but since we're getting rid of the get_type() in input we can just re-use it's enum (which was the same except with one more member, which I deleted here). The good news is this will just disappear once we merge this in to #598, so the actual diff will be negative in terms of LOC!
| InputType::Json => Err(ValError::InternalErr(PyNotImplementedError::new_err( | ||
| "Cannot check isinstance when validating from json,\ | ||
| use a JsonOrPython validator instead.", | ||
| ))), |
There was a problem hiding this comment.
Happy to do something else here. We, unfortunately, have to do this at runtime because we build a single validator for all of the options. Since we have StrictOrLax schema and JsonOrPython we may want to eventually re-evaluate that choice which would let us do optimizations like inlining those two branches and doing this check statically instead of during validation, which would lead to a better user experience.
| #[derive(Debug, Clone, Copy)] | ||
| pub enum ValidationMode { | ||
| Json, | ||
| Python, | ||
| } | ||
|
|
||
| impl<'source> FromPyObject<'source> for ValidationMode { | ||
| fn extract(ob: &'source PyAny) -> PyResult<Self> { | ||
| match ob.extract::<&str>()? { | ||
| "json" => Ok(ValidationMode::Json), | ||
| "python" => Ok(ValidationMode::Python), | ||
| other => Err(PyTypeError::new_err(format!( | ||
| "Expected one of 'json' or 'python', no validation mode '{other}'" | ||
| ))), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl IntoPy<PyObject> for ValidationMode { | ||
| fn into_py(self, py: Python<'_>) -> PyObject { | ||
| match self { | ||
| Self::Json => intern!(py, "json").into(), | ||
| Self::Python => intern!(py, "python").into(), | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
See comment above, this won't be in the final diff.
This removes the now dead code from #598 according to the uprev in pydantic