CU-8695d4www pydantic 2 #476
Conversation
…e on meta cat and tner
|
Task linked: CU-8695d4www Support pydantic 2? |
|
NOTE: Though the issue is that any future changes would probably not be tested in any capacity on pydantic 1 (v2 will be installed by default). |
…sts in deprecation method
baixiac
left a comment
baixiac
left a comment
There was a problem hiding this comment.
Nice work. Just that I noticed pydantic v1 dict() is still called on the MultiDescriptor base model at
| auto_save_model: bool = True | ||
| """Should do model be saved during training for best results""" | ||
| last_train_on: Optional[int] = None | ||
| last_train_on: Optional[float] = None |
medcat/utils/decorators.py
Outdated
| def deprecated(message: str, depr_version: Tuple[int, int, int], removal_version: Tuple[int, int, int]) -> Callable: | ||
| def deprecated(message: str, depr_version: Tuple[int, int, int], | ||
| removal_version: Tuple[int, int, int], | ||
| allow_usage: bool = False) -> Callable: |
There was a problem hiding this comment.
Maybe there is no need to add and expose this argument to the public API? It looks deprecated() will be monkey-patched before tests run anyway.
There was a problem hiding this comment.
The method does indeed get monkey-patched during testing.
I suppose we could add **kwargs to the method to avoid describing the argument. But we can't remove it entirely since then the addition of the allow_usage keyword/argument would cause an exception during use (TypeError due to unexpected keyword argument).
But at the same time, we if this was hidden, the future maintainer of this resource might not know how to achieve the same result.
The main reason the method gets monkey-patched during testing is because we (generally) want to avoid using deprecated methods in our code. And if we had 100% test coverage (which we certainly don't - but that's besides the point), raising an exception during test time would guarantee that we're not.
But I've made an exception to this one, mostly so we can pre-specify when we'd stop supporting pydantic 1. So that we don't forget to do this at this later date, and so that it's actually documented (at least in code) and caught during GHA workflow (at the appropriate release time).
But if you've got some ideas on how to do this more elegantly, then don't hesitate to propose them!
There was a problem hiding this comment.
Alright. In that case, I think there is no need to deprecate get_model_dump and get_model_fields in this PR given the goal is to support both versions ATM and they are supposed to do the job well. Besides, it is a bit weird to add a new method and deprecate it at the same time while I have no strong opinion on this.
There will be warnings like Field "model_X" has conflict with protected namespace "model_" so to me, extra base model field renaming will be needed before medcat fully embraces pydantic v2 and deprecates v1.
…oning utils for typing during GHA workflow
adam-sutton-1992
left a comment
adam-sutton-1992
left a comment
There was a problem hiding this comment.
LGTM - As discussed maybe a beta test is suitable for such a large sweeping change which is difficult to completely test as developers.
* CU-8695d4www: Bump pydantic requirement to 2.6+ * CU-8695d4www: Update methods to use pydantic2 based ones * CU-8695d4www: Update methods to use pydantic2 based ones [part 2] * CU-8695d4www: Use identifier based config when setting last train date on meta cat and tner * CU-8695d4www: Use pydantic2-based model validation * CU-8695d4www: Add workarounds for pydantic1 methods * CU-8695d4www: Add missing utils module for pydantic1 methods * Revert "CU-8695d4www: Bump pydantic requirement to 2.6+" This reverts commit b0b3d431cc01e2e73c8708bd007e7e948263deb9. * CU-8695d4www: [TEMP] Add type-ingores to pydantic2-based methods for GHA workflow * CU-8695d4www: Make pydantic2-requires getattribute wrapper only apply when appropriate * CU-8695d4www: Fix missin model dump getter abstraction * CU-8695d4www: Fix missin model dump getter abstraction (in CAT) * CU-8695d4www: Update tests for pydantic 1 and 2 support * Revert "CU-8695d4www: [TEMP] Add type-ingores to pydantic2-based methods for GHA workflow" This reverts commit b86135add8c8ad944a83c0c51f425a3e55d940e7. * Reapply "CU-8695d4www: Bump pydantic requirement to 2.6+" This reverts commit 080ae7172434a849e81ae0662d9310588f4bb9a3. * CU-8695d4www: Allow both pydantic 1 and 2 * CU-8695d4www: Deprecated pydantic utils for removal in 1.15 * CU-8695d4www: Allow usage of specified deprecated method(s) during tests * CU-8695d4www: Allow usage of pydantic 1-2 workaround methods during tests * CU-8695d4www: Add documentation for argument allowing usage during tests in deprecation method * CU-8695d4www: Fix allowing deprecation during test time * CU-8695d4www: Fix model dump getting in regression checker * Revert "CU-8695d4www: Fix allowing deprecation during test time" This reverts commit fadc7d18e695e8217e26e5191ff11e36dac44fde. * Revert "CU-8695d4www: Add documentation for argument allowing usage during tests in deprecation method" This reverts commit 927f8078083f1cc033776aa1407a424ac5601391. * Revert "CU-8695d4www: Allow usage of pydantic 1-2 workaround methods during tests" This reverts commit 825628e10db04b45143e8ec84af71781a04d7725. * Revert "CU-8695d4www: Allow usage of specified deprecated method(s) during tests" This reverts commit a89e6804e2d533cc21e4409d0045bb7c63bf743f. * Revert "CU-8695d4www: Deprecated pydantic utils for removal in 1.15" This reverts commit 0ee1a8abc3fa429beb3094c4ff465876d41677e6. * CU-8695d4www: Add comment regarding pydantic backwards compatiblity where applicable * CU-8695d4www: Add pydantic 1 check to GHA workflow * CU-8695d4www: Fix usage of pydantic-1 based dict method in regression results * CU-8695d4www: Fix usage of pydantic-1 based dict method in regression tests * CU-8695d4www: New workflow step to install and run mypy on pydantic 1 * CU-8695d4www: Add type ignore comments to pydantic2 versions in versioning utils for typing during GHA workflow * CU-8695d4www: Update pydantic requirement to 2.0+ only * CU-8695d4www: Update to pydantic 2 ONLY * CU-869671bn4: Update mypy dev requirement to be less than 1.12 * CU-869671bn4: Fix model fields in config * CU-869671bn4: Fix stats helper method - use correct type adapter * CU-869671bn4: Fix some model type issues * CU-869671bn4: Line up with previous model dump methods * CU-869671bn4: Fix overwriting model dump methods * CU-869671bn4: Remove pydantic1 workflow step
Let's see if we can do both pydantic 1 and pydantic 2.
This should in principle work with both.
Though I don't want to run GHA for both all the time. So I'll first run it for pydantic 2, then (if successful), change back to 1 and run it on that. And (if that's all good), change back to pydantic 2.
EDIT:
I've now added something to the main GHA workflow that should catch any use of the
.dict(method or the.__fields__attribute from the pydantic1 era.However, this is only limited to those two (the only things that were previously being used). So if something else that doesn't work cross 1 and 2 is used, it wouldn't be caught.
This could also at some point match other use cases unrelated the pydantic since it's a simple grep (it doesn't currently).
EDIT#2:
TODO: Support pydantic 2 only