Attempt to upgrade to pydantic 2 #291
Conversation
maxkahan
left a comment
maxkahan
left a comment
There was a problem hiding this comment.
Hi, this is a great contribution and I would say almost there in terms of merging and releasing this. There are some issues that hold it back a little though, that we'd need to fix to get it over the line.
- There are quite a few warnings about the
validatedecorator (from v1) and various objects (includingsmart_union) that have also been deprecated/are discouraged. - Methods like
parse_obj,dictetc. are deprecated as well. - Class-based config is deprecated so this is throwing a warning.
- There seems to be a couple of issues with serialization.
- Finally, you don't need to use the
Optionaltype to show optional model arguments any more, Pydantic infers this now. Not a big deal though, this one isn't a must-fix, just makes the code tidier.
The warnings are coming through to test scripts showing real SDK use cases, so they do need to go really before we can release this, but this is a great start that manages to get quite a lot of the way there!
In terms of getting to a release, I think it's just a bit of swapping more old syntax for new v2 syntax and changing a couple of model fields around.
|
My apologies I was running I was able to go through and address all the pydantic deprecations. I think it's much better now!
Let me know if there's anything else you need |
| # nbs.get_pay_text_prompt(), | ||
| ] | ||
| ncco = Ncco.build_ncco(actions=action_list) | ||
| assert ncco == nbs.insane_ncco |
There was a problem hiding this comment.
If we could get this one passing I think it's good!
|
Hi @DomThePorcupine this is awesome, thanks for working on the changes so quickly. Just the one test you commented out that we should have back in, but everything else is looking great! 🙂 |
|
Whoops! That was embarrassing, thanks for catching that :) |
| type = Field('sip', const=True) | ||
| type: Literal['sip'] = 'sip' | ||
| uri: str | ||
| headers: Optional[dict] |
There was a problem hiding this comment.
Have had a final read through and it looks like this needs an = None to mark it optional, just removing the Optional type here has made it required. I think there may be more cases like this where removing the Optional type has made it a required field 😖
There was a problem hiding this comment.
Update: have consulted the pydantic docs and Optional has a specific meaning in this case. I think it's okay to swap it out for = None here though as when we serialize models we ignore None values anyway
| uri: AnyUrl | ||
| contentType: Literal['audio/l16;rate=16000', 'audio/l16;rate=8000'] | ||
| headers: Optional[dict] | ||
| headers: dict |
There was a problem hiding this comment.
| headers: dict | |
| headers: dict = None |
| class VoicePrompt(BaseModel): | ||
| language: Optional[str] | ||
| style: Optional[int] | ||
| language: str |
There was a problem hiding this comment.
| language: str | |
| language: str = None |
| language: Optional[str] | ||
| style: Optional[int] | ||
| language: str | ||
| style: int |
There was a problem hiding this comment.
| style: int | |
| style: int = None |
There was a problem hiding this comment.
I also tabbed through - only found one other place where the None was missing
|
Think I found all the parts that needed changing, but it's probably wise to double check |
|
|
||
| class PayPrompts: | ||
| class VoicePrompt(BaseModel): | ||
| language: Optional[str] |
There was a problem hiding this comment.
Also caught one here
maxkahan
left a comment
maxkahan
left a comment
There was a problem hiding this comment.
Looks like we've worked out all the kinks! Thanks for your work on this PR, I'll merge it in and prepare for a new SDK release 😄
2 participants
I missed that someone else already had a PR open for this :( but I did manage to get all the tests passing with minimal changes, so I'm opening this in case it's helpful :)