pydantic2.0 #168

ruthenian8 · 2023-07-14T13:21:34Z

Description

Use pydantic 2.0

Checklist

I have covered the code with tests
I have added comments to my code to help others understand it
I have updated the documentation to reflect the changes
I have performed a self-review of the changes

… using the 'singledispatchmethod' decorator

RLKRo · 2023-07-25T22:45:33Z

dff/context_storages/json.py


 from .database import DBContextStorage, threadsafe_method
 from dff.script import Context


-class SerializableStorage(BaseModel, extra=Extra.allow):
-    @root_validator
+class SerializableStorage(BaseModel, extra="allow"):


Not directly related to pydantic 2.0, but why is this storage implemented using this class?
Seems like the only use is to cast all values in loaded json file to Context but can't we just do that inside the _load method?
If not, this class should be documented and defined inside the JSONContextStorage class.

Previously this issue wasn't that evident, but now code like len(self.storage.model_extra) looks weird.

I don't think that changes to this class are necessary, since it's going to be replaced by the new context storage implementation

dff/messengers/telegram/message.py

dff/pipeline/types.py

RLKRo · 2023-07-25T23:18:17Z

dff/script/core/context.py

        elif not issubclass(type(ctx), Context):
            raise ValueError(
                f"context expected as sub class of Context class or object of dict/str(json) type, but got {ctx}"
            )
        return ctx

-    @validate_arguments


What's the status on the pickling issue?
Is it going to be fixed in #93?

validating arguments is not needed now, we can parse the argument inside the function body

We can but that is not how we are supposed to work with pydantic.

In my view, this strategy is better, since it does not modify the Context model, while using validate_call decorators leads to the Message class being referenced in the Context class body which causes the pickling problems

dff/script/core/context.py

dff/context_storages/json.py

dff/script/core/message.py

dff/script/core/script.py

RLKRo · 2023-07-26T00:18:09Z

dff/script/__init__.py

    normalize_label,
    normalize_condition,
-    normalize_transitions,
    normalize_response,
    normalize_processing,
-    normalize_script,
 )


Not related to the PR, but:

Why import these functions if they are not supposed to be used by the user?

https://trello.com/c/vKJsysZq

RLKRo

This PR seems good but

There are still three comments left that are unrelated to this PR:

pydantic2.0 #168 (comment) -- question about json storage for @pseusys
pydantic2.0 #168 (comment) -- question about context pickling (why do we pickle all methods of the context class and is this behavior going to be changed) for @pseusys
pydantic2.0 #168 (comment) -- question about function imports for @kudep

RLKRo · 2023-07-26T11:07:18Z

dff/script/core/context.py

        elif not issubclass(type(ctx), Context):
            raise ValueError(
                f"context expected as sub class of Context class or object of dict/str(json) type, but got {ctx}"
            )
        return ctx

-    @validate_arguments


We can but that is not how we are supposed to work with pydantic.

pseusys · 2023-08-02T11:59:05Z

Speaking of the comment:

Json storage is using BaseModel and to my mind not using it won't be easier for several reasons (otherwise custom json serializer functions will be required):
- Context class includes nested timestamp objects and json module doesn't know how to serialize them.
- There are Dicts with integer keys in Context objects that can't be serialized by json module.
Pickle modlue serializes only the required fields now, it works as expected.

RLKRo · 2023-08-02T13:05:41Z

Pickle modlue [sic] serializes only the required fields now, it works as expected.

"now" means in #93?

I'm assuming this PR will be merged before that one, so I think we should return validate_call decorators for context methods inside the partial_context_updates branch (once this one is merged).

pseusys · 2023-08-02T17:08:28Z

Yes, "now" means in #93.
I don't really think we should wrap anything in validate_all in partial context updates branch since no untrusted data is pickled or unpickled.

RLKRo · 2023-08-02T17:15:26Z

validate_call validates types of arguments.

Previously it was validate_arguments, pydantic 2.0 changed it.
New version validate_call wasn't working with pickle, so we used manual validation (request = Message.model_validate(item)), which is supposed to happen inside the validate_call decorator.

Or do you mean that we shouldn't use any type validation? I don't think we should do that. Type validation would help in case a user would accidentally write a function that sets ctx.last_request to str instead of Message.

pseusys · 2023-08-02T17:45:28Z

I only meant that I don't think there should be any concerns about pickling while reasoning about validating arguments.
I don't really understand how pickling should affect argument validation.

ruthenian8 added 2 commits July 14, 2023 16:19

initial commit

922f8a9

update setup.py

cbce46d

ruthenian8 changed the title ~~initial commit~~ pydantic2.0 Jul 14, 2023

ruthenian8 added 5 commits July 14, 2023 18:24

Uncomment validate_call for context methods; avoid pydantic errors by…

9a58516

… using the 'singledispatchmethod' decorator

update version requirements to pydantic 2.0.3

b8760f9

Comment validate_call temporarily

d237195

bypass lint problem

ae9b3e5

remove commented references to validate_call

f67d242

kudep marked this pull request as ready for review July 25, 2023 11:27

RLKRo requested changes Jul 26, 2023

View reviewed changes

Update model configurations, update property setters for Context

45010ee

RLKRo approved these changes Jul 26, 2023

View reviewed changes

RLKRo added the dependencies Updating dependencies label Jul 26, 2023

RLKRo assigned ruthenian8 Jul 26, 2023

Merge branch 'dev' into feat/pydantic2.0

32a41c2

kudep approved these changes Aug 3, 2023

View reviewed changes

kudep merged commit 72e167f into dev Aug 3, 2023

kudep deleted the feat/pydantic2.0 branch August 3, 2023 13:28

RLKRo mentioned this pull request Sep 18, 2023

Partial context updates #93

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pydantic2.0 #168

pydantic2.0 #168

ruthenian8 commented Jul 14, 2023

RLKRo Jul 25, 2023

ruthenian8 Jul 26, 2023

ruthenian8 Jul 27, 2023

RLKRo Jul 25, 2023

ruthenian8 Jul 26, 2023

RLKRo Jul 26, 2023

ruthenian8 Jul 26, 2023

RLKRo Jul 26, 2023

kudep Jul 28, 2023

RLKRo left a comment

RLKRo Jul 26, 2023

pseusys commented Aug 2, 2023

RLKRo commented Aug 2, 2023

pseusys commented Aug 2, 2023

RLKRo commented Aug 2, 2023

pseusys commented Aug 2, 2023

pydantic2.0 #168

pydantic2.0 #168

Conversation

ruthenian8 commented Jul 14, 2023

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RLKRo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pseusys commented Aug 2, 2023

RLKRo commented Aug 2, 2023

pseusys commented Aug 2, 2023

RLKRo commented Aug 2, 2023

pseusys commented Aug 2, 2023