gdsfactory / kfactory

gdsfactory with a klayout backend
https://gdsfactory.github.io/kfactory/
MIT License
31 stars 11 forks source link

pydantic < 2 #172

Closed joamatab closed 1 year ago

joamatab commented 1 year ago

Could we move to pydantic<2 until all the other tools have moved? (tidy3d ...)

sebastian-goeldi commented 1 year ago

I checked the tidy3d issue. If I see the isse there, it seems to be a problem of pylint. What's the strategy for migrating then?

It seems pylint has no intention of fixing this, and it's not a coding reason that is the hold up.

If there is no plan of any of the packages involved to fix it, I don't see the benefit of reverting to pydantic1. I don't intend to keep a package on a version that is just on life support.

joamatab commented 1 year ago

@tylerflex

sounds good, i hope tidy3d moves soon to pydantic2 then

tylerflex commented 1 year ago

Here's a summary:

  1. pylint doesn't properly recognize types of fields in pydantic 2.0. This is true when importing pydantic or pydantic.v1. So for example a

    var: int = pd.Field()

    gets parsed by pylint as a FieldInfo not an int. If you use self.var as an int in the codebase, pylint will complain directly. It also affects pylint's ability to reason about var in tons of ways.

  2. Many of tidy3d internals depend on the pydantic 1. conventions. It seems 2. has changed a lot and it's a significant effort to port our code to 2.* and test it to ensure that our customers don't encounter errors.

There are a few solutions:

A. Keep pydantic at 1. until we release a new major version of tidy3d 3.0 in several months / early next year. This new version would be written from scratch in pydantic 2. or some other package. B. Remove pylint from our development process and start using pydantic.v1 imports internally. We can at least disable any pylint warning related to the issue #1 above, of which there are thousands.. We can supplement with ruff although that is proving to be a bit not ideal as we have been trying it out lately. C. Complete rewrite of tidy3d using pydantic 2.*, which I've attempted but we need a lot of time to test this before I'm comfortable releasing it.

My top priority is making sure that users don't run into any errors so I want to be very conservative about upgrading. I'm not super confident about pydantic 2. and 1. has been working for us fine. In that sense, I am most in favor (all else equal) with option A, B, C. in that order.

I'm a bit disappointed that neither pylint nor pydantic wanted to fix this issue as we rely on both packages quite a bit and I'm sure many others do as well. Do you have any suggestions or thoughts?

sebastian-goeldi commented 1 year ago

I am much more in favor of B.

And to be honest, I am not super happy that pylint just seems to not consider this a bug. If it is about type checking, I would suggest to use mypy instead. Pydantic has a native plugin for mypy, which kfactory uses https://github.com/gdsfactory/kfactory/blob/main/pyproject.toml#L108 and this does not cause any issues with type recognition even when using Fields like for example here https://github.com/gdsfactory/kfactory/blob/main/src/kfactory/utils/enclosure.py#L1098C49-L1098C49 . I think this is not really a problem on pydantic's side, so I understand their stance that it's not a pydantic problem. But I think using mypy makes some sense anyway, as it is the reference implementation. But I also understand that it might be harder to configure the full testing suite, as mypy is a rather specific tool and will probably not cover everything that pylint does. I can highly recommend the pyproject.toml style in general though, as you e.g. don't need the requirements folder anymore (or any requirements.txt for that matter) and almost all bigger tools support pyproject.toml configurations by now.

I totally understand not wanting to break the package for customers, hence the recommendation for pre-releases which would require specific pinning of those version(s).

tylerflex commented 1 year ago

Thanks @sebastian-goeldi option B is currently in testing in this PR: https://github.com/flexcompute/tidy3d/pull/1035. Tests pass because I removed pylint and replaced with a very basic ruff, but we still need to decide as a team if this is sufficient and are playing with different ruff configurations. here: https://github.com/flexcompute/tidy3d/pull/1053

Can you elaborate on the pre-release idea? we already do pre-releases for specific features. Are you suggesting we do a pre-release supporting pydantic 2.* along with every release? or periodically?

tylerflex commented 1 year ago

I could do a pre-release already of the PR I just linked and gdsfactory could pin to that version, but maintaining that seems like a pain if gdsfactory wants to stay up to date with new features.

sebastian-goeldi commented 1 year ago

Yes, I think periodically should be sufficient in my opinion, and not do a 1->2 conversion until you have rewritten it for tidy3d 3.0. If you are planning to rewrite the whole package, I think it wouldn't make sense to convert to 2. syntax now, but rather use the pydantic.v1 imports. And then use the pydantic 2. syntax for 3.0

I cannot really speak for gdsfactory, but imo it should be sufficient to make a periodic release with the .v1 backport. Unless I am mistaken about some feature changes for pydantic.v1 in 2.*, it shouldn't be a huge problem to maintain that, it should be sufficient to do a rebase (hopefully) with main/master. I also think gdsfactory normally doesn't need the latest latest release unless there was a breaking bug. But I don't really want to give promises there and will let @joamatab speak for that part.

If you need some help, I might be able to help a bit in that regard for testing and fixing some basic bugs, as I was looking to integrate it more with kfactory through kplugins , as PsiQ is looking into using tidy3d more.

My main concern with staying on 1.* for another year is that by then the migration path might not be that straight forward anymore (espeically since kfactory uses some features that are now their own separate package from pydantic such as Color and BaseSettings.

Wrt ruff, I think ruff should give you almost everything you need. Considering their very impressive project list that are using ruff, I hope can cover your use cases. I tested it for kfactory as well, but at that time rustpython was still lacking some 3.10 syntax support, maybe I should reevaluate using it for kfactory. I think my main concern was ruff-lsp lacking some base lsp stuff like definition lookup etc.

Ruff is extremely actively developed and used in major open-source projects like:

Apache Airflow FastAPI Hugging Face Pandas SciPy

tylerflex commented 1 year ago

Thanks @sebastian-goeldi after a bit of initial resistance I think I finally got ruff configured in a way that makes sense for us and replaces pylint. I am definitely impressed by ruff and think it's the proper way forward for our project.

I also added an issue to use pyproject.toml for fully configuring tidy3d, I think that's a good idea just haven't gotten around to it.

This PR introduces pydantic 2.* support for tidy3d, we should have it out in a couple weeks @joamatab, assuming we don't run into any issues. We also probably need to do backend work, which could take a bit of time.

Ultimately I think this is a win win. We can all upgrade our pydantic requirements and ruff seems like a better tool for the future anyway. Thanks for the discussion!

tylerflex commented 1 year ago

FYI: we just merged pydantic v2 in tidy3d. We'll release it soon under our pre-release 2.4.0rc2 and then in our stable 2.4.0 release.

sebastian-goeldi commented 1 year ago

So, pydantic 2.0 gdsfactory soon @joamatab ? ;)

joamatab commented 1 year ago

Yay, looking forward to the next release of tidy3d

now we need meow and SAX :)

@flaport

flaport commented 1 year ago

It's on my todo list for next week!

joamatab commented 1 year ago

This is pretty helpful

https://github.com/pydantic/bump-pydantic#installation

took me 90% there

flaport commented 1 year ago

SAX and MEOW are ported using pydantic.v1 for now. For SAX you can use >=0.9.0 as a dependency. For meow it will be >=0.8.0, but I will only release the meow version once tidy3d makes its 2.4 release.

Next I will port SAX to full pydantic v2 and then meow as well (the latter might be a bit more difficult).

sebastian-goeldi commented 1 year ago

@joamatab this should no longer be an issue, so can we close this?

joamatab commented 1 year ago

Yes, we are all now pydantic2 compatible :)