Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pydantic V2 #313

Closed
sinisaos opened this issue Jun 22, 2023 · 27 comments
Closed

Pydantic V2 #313

sinisaos opened this issue Jun 22, 2023 · 27 comments

Comments

@sinisaos
Copy link
Member

sinisaos commented Jun 22, 2023

Since the Piccolo ecosystem depends on Pydantic, the new Pydantic V2 beta is out and libraries like FastAPI are available for testing. V2 brings a lot of braking backwards compatibility changes and I wanted to see how much work we'll have once we switch to V2. I managed to change the code and tests in Piccolo ORM and Piccolo API and everything works (some DeprecationWarning are left). The most changes are in Piccolo Admin, as expected, because the schema has changed (anyOf keyword which we don't use etc.). Now the properties fields are arranged alphabetically (as well as Piccolo Admin form fields which is a bit strange), but I see that this has changed and that everything will be like in V1 (this merged PR). Mostly everything works except:

  • DurationWidget (Interval) - fixed
  • FlatPickr (datetime widget) - fixed
  • ArrayWidget (arrays and file-multifile upload) - arrays works 😄
  • nullable columns - fixed

mostly due to changes in schema. I'm sorry for the long comment, but I hope this will be useful and a placeholder for later changes we will need to make when we need to move to V2.

@dantownsend
Copy link
Member

@sinisaos Thanks for looking into it. It's a shame about the breaking changes to the schema - but we should be able to work around it. I wonder if the schema has some kind of version number in it.

@sinisaos
Copy link
Member Author

@dantownsend When the time comes, we'll probably be able to fix everything. There is now an anyOf keyword in the schema and it contains the formats if they are not defined in Piccolo pydantic.py. The textarea is fine because

elif isinstance(column, Text):
    field = pydantic.Field(format="text-area", extra=extra, **params)

textarea

but for example date format is in the anyOf keyword and the Piccolo admin cannot currently read it from the schema.

datetime

I don't know if this is a good solution, but this way I was able to fix the nullable columns like this

export function convertFormValue(params: {
    key: string
    value: any
    schema: Schema
}): any {
    let { key, value, schema } = params

    if (value == "null") {
        value = null
    } else if (schema.properties[key]["anyOf"][0].type == "array") {
        value = JSON.parse(String(value))
    } else if (schema?.properties[key]["anyOf"][0].format == "uuid" && value == "") {
        value = null
    } else if (schema?.properties[key]["anyOf"][0].format == "email" && value == "") {
        value = null
    } else if (schema?.properties[key]["anyOf"][0].format == "date-time" && value == "") {
        value = null
    } else if (schema?.properties[key]["anyOf"][0].type == "integer" && value == "") {
        value = null
    } else if (schema?.properties[key]["anyOf"][0].type == "number" && value == "") {
        value = null
    } else if (
        schema?.properties[key].extra.foreign_key == true &&
        value == ""
    ) {
        value = null
    }
    return value
}

@sinisaos
Copy link
Member Author

@dantownsend ArrayWidget it works though. I think the problem with the DurationWidget is that Pydantic V2 changed the timedelta from this https://github.com/pydantic/pydantic/blob/e60c5c0a7e37f4a2caaeaac320a2fa3e28c7a14d/pydantic/v1/schema.py#L780
to this https://github.com/pydantic/pydantic/blob/e60c5c0a7e37f4a2caaeaac320a2fa3e28c7a14d/pydantic/json_schema.py#L543
so that it no longer uses the type:integer and the format:time-delta

@dantownsend
Copy link
Member

OK, so the schema for timedelta now says:

{'type': 'string', 'format': 'duration'}

And before it was:

{'type': 'number', 'format': 'time-delta'}

I wonder why they decided to change it? I wonder if there's any significance in JSON schema?

Pydantic currently serialises a timedelta into a number:

https://demo1.piccolo-orm.com/api/tables/movie/1/?__readable=true

{
  "id": 1,
  "name": "Star Wars: Episode IV - A New Hope",
  "rating": 93.3,
  "duration": 7500.0, // <- THIS
  // ...
}

I wonder what the new string representation looks like - do you know?

@sinisaos
Copy link
Member Author

I wonder why they decided to change it? I wonder if there's any significance in JSON schema?

Here's a issue for that.

{
  "id": 1,
  "name": "Star Wars: Episode IV - A New Hope",
  "rating": 93.3,
  "duration": 7500.0, // <- THIS
  // ...
}

I wonder what the new string representation looks like - do you know?

This is new representation

{
  "id": 1,
  "name": "Star Wars: Episode IV - A New Hope",
  "rating": 93.3,
  "duration": "PT7500S", // <- THIS
  // ...
}

@dantownsend
Copy link
Member

@sinisaos Interesting - I've never seen a format like PT7500S before ...

From the docs it looks like it means:

  • P = duration
  • T = time
  • 7500S = 7500 seconds.

Seems like we'll need some kind of parser for it.

@sinisaos
Copy link
Member Author

@dantownsend Flatpickr widget and DurationWidget have been fixed and can be used. Now everything works as it should, and later we can tweak everything a bit more. For the DurationWidget, I used moment.js to parse and convert beetwen period duration values and seconds ​​(which we use). I think it is much better to use an external library than to write own parser because it is not easy to convert beetwen formats without mistakes. The readableInterval function for humanReadable filter in RowListing.vue has also been fixed. For now we are ready to move to Pydantic V2 when it is in alpha and if there are any changes we will be able to make them.

@dantownsend
Copy link
Member

Thanks for that.

It's worth taking a look at alternatives to moment.js:

https://momentjs.com/docs/#/-project-status/

Even though it's still commonly used, it's not recommended for new projects. One of the reasons is it's not 'tree shakeable' so is quite a large dependency.

@sinisaos
Copy link
Member Author

@dantownsend Moment.js is one of the most popular libraries. In our case, there is a very convenient methods to convert the period duration to seconds

// convert ISO 8601 duration string (e.g. `P17DT14706S`) to seconds
moment.duration(timeValue).asSeconds()

and vice versa

// convert seconds to ISO 8601 duration string (e.g. P17DT14706S)
moment.duration(value, 'seconds').toISOString()

I will see if there is an equally good alternative. Everything is subject to change and is just a test of how much work there is when we move to Pydantic V2.

@dantownsend
Copy link
Member

I wonder how simple that moment.duration(timeValue).asSeconds() method is - does it require all of the moment internals to work, or can it be adapted to our use if it's just a few lines of code.

@sinisaos
Copy link
Member Author

@dantownsend Sorry, but I haven't found any equivalent Moment.js alternatives so far. When the time comes, feel free to try to write an alternative, but it's tricky. I tried with a regular expression, but I was getting wrong results because I couldn't properly convert the weeks and days along with the seconds that we need. I will push the changes to Github tomorrow (and paste links here) so we can have a reference for later and you can take a look at what I've done so far.

@sinisaos
Copy link
Member Author

sinisaos commented Jun 24, 2023

@sinisaos
Copy link
Member Author

@dantownsend I found a library TinyDuration that is really small and should meet our parsing and conversion needs. I just need to try it.

@dantownsend
Copy link
Member

@sinisaos Cool, thanks a lot for that.

That library looks promising. So the format is ISO-8601 - I thought it was just a JSON schema specific thing, but if it's a proper standard that's good.

@sinisaos
Copy link
Member Author

@dantownsend Sorry, but I haven't been able to get TinyDuration to work because there's some weird conversion between seconds and duration (and vice versa) that I haven't been able to sort out for our needs. I know that Moment.js (bundle size) is a big dependency, but it's not much bigger than e.g. vue2-editor (bundle size), but Moment.js has everything we need and everything works as it should (conversion between seconds and duration is done properly). I also fixed the timedelta in the DurationWidget to work with the new one interval format and custom forms also work now.

@dantownsend
Copy link
Member

@sinisaos What's the problem with parsing seconds then?

I had a quick play around, and TinyDuration seems to handle simple use cases:

>>> tinyduration.parse('PT6S')
{seconds: 6}

>>> tinyduration.serialize({'seconds': 6})
'PT6S'

@sinisaos
Copy link
Member Author

@dantownsend Everything works fine with that simple example, but we also have years and days, but that's not a problem either, and I managed to solve that with this in readableInterval.

export function readableInterval(timeValue: string) {
    let timeRange = 0
    let parsedTimeValue = parse(timeValue)
    if (parsedTimeValue["years"]) {
        timeRange += parsedTimeValue["years"] * 31536000
    }
    if (parsedTimeValue["days"]) {
        timeRange += parsedTimeValue["days"] * 86400
    }
    if (parsedTimeValue["seconds"]) {
        timeRange += parsedTimeValue["seconds"]
    }
   ....

The problem is parsing and serializing later. There are two videos that show exactly what I mean.
With Moment.js everything is fine, even timedelta in DurationWidget.

moment.mp4

With TinyDuration is not.

tinyduration.mp4

If we try to edit the existing record, format PTPT9840SS is not valid and should be PT9840S, and when manually stripped (removed the redundant PT and S), the record can be saved and shows a good human readable format in RowListing.vue, It's even worse when we want to adding a new record when format is PTPS which is not valid (valid duration for 0 seconds is either PT0S or P0D as in Moment.js). When we try to save that record we get invalid duration because duration:"P" is saved in database which is not correct. Timedelta also doesn't work. I tried everything from setting a default value, slicing redundant PT and S, if-else statements on adding and editing records, if-else statements on convertFormValue to save the good values ​​to the database, but nothing worked in my case. I'm sorry. I hope you can do better (we can always use Moment.js which works fine) and I hope I explained well what the problem is.

@dantownsend
Copy link
Member

@sinisaos Thanks for sharing those videos. I wonder why TinyDuration is adding those extra prefixes to the string 🤔

Just use moment.js for now then. I can always take a look at it later on, and switch to TinyDuration if I can figure it out.

@sinisaos
Copy link
Member Author

sinisaos commented Jul 7, 2023

Piccolo ORM - piccolo-orm/piccolo@master...sinisaos:piccolo:pydanticV2_test
Piccolo API - piccolo-orm/piccolo_api@master...sinisaos:piccolo_api:pydanticV2_test
Piccolo Admin - master...sinisaos:piccolo_admin:pydanticV2_test

@dantownsend I have completed the changes to make the Piccolo ecosystem work with Pydantic V2 and you can see it on the links above. Everything works, all tests pass and there are no more deprecated warnings. I tried to change the codes in Piccolo API and Piccolo Admin as little as possible. Feel free to try it and make review.

@dantownsend
Copy link
Member

@sinisaos Yeah, I just realised Pydantic v2 has already been released. So it's pretty urgent now. I was hoping to finish the charts first, but this is more important.

In the short term we need to version pin Pydantic / FastAPI in the Piccolo, Piccolo API and Piccolo Admin repos ASAP, otherwise they won't work any more. At least there will be a version which still works correctly with Pydantic v1, FastAPI < v0.100.0. Can you help me with this if you get a chance?

And then next week we will have to bump the major version on each of the Piccolo repos, as the Pydantic v2 is such a big breaking change.

@sinisaos
Copy link
Member Author

sinisaos commented Jul 7, 2023

@dantownsend For Piccolo, I have already pinned Pydantic to 1.10.11, which is the last version before V2 in this PR. If you want I can make a PR to pin FastAPI==0.99.0 in requirements.txt to Piccolo API and Piccolo Admin now. Starting Monday next week I'm going out of town with my wife and kids for 2-3 weeks, so I won't have my laptop, so unfortunately I won't be of much help until I get back.

@dantownsend
Copy link
Member

@sinisaos No worries, hope you have a great holiday!

If you don't mind creating a PR to pin Piccolo API and Piccolo Admin I'll release them tonight.

I'll review the Pydantic v2 PRs next week and will release them asap.

@sinisaos
Copy link
Member Author

sinisaos commented Jul 7, 2023

@dantownsend Thanks. I'll check my email when I can and get back to you if I can be of any help.
PRs for Piccolo Admin and Piccolo API are ready.
You can also merge the Litestar template update PR and with that PR the Piccolo main repo will be pinned to Pydantic 1.10.11 and everything should be fine.

@dantownsend
Copy link
Member

I've reviewed all the PRs for this - thanks a lot, I can see it was a lot of work.

I'm going to release an alpha version of Piccolo 1.0 to PyPI, otherwise we can't get a PR passing for Piccolo API or Piccolo Admin.

The order of the alpha releases will have to be Piccolo -> Piccolo API -> Piccolo Admin.

@sinisaos
Copy link
Member Author

@dantownsend Great. That sounds like a good plan. I will follow the progress as much as I can and try to be as helpful as I can since I am without a laptop.

@sinisaos sinisaos closed this as not planned Won't fix, can't repro, duplicate, stale Oct 10, 2023
@dantownsend
Copy link
Member

@sinisaos Sorry for the delay on this. Have been really busy with family stuff - I will get this released this week I promise.

I can probably still recover the work from a local branch, if you can't reopen this PR.

@dantownsend dantownsend reopened this Oct 10, 2023
@dantownsend
Copy link
Member

@sinisaos This has now been released - thanks for all your help with this.

I've upgraded one of my apps, and it seems OK so far.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants