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 2 - migration #279

Merged
merged 31 commits into from
Jan 15, 2024
Merged

pydantic 2 - migration #279

merged 31 commits into from
Jan 15, 2024

Conversation

bertiqwerty
Copy link
Contributor

@bertiqwerty bertiqwerty commented Sep 7, 2023

The goal of this MR is to upgrade to Pydantic 2. For the initial commit I run bump-pydantic and changed some things manually. There are still some open issues, though.

@bertiqwerty bertiqwerty changed the title first steps towards migration pydantic 2 - first steps towards migration Sep 7, 2023
@bertiqwerty bertiqwerty changed the title pydantic 2 - first steps towards migration pydantic 2 - migration Sep 7, 2023
This was referenced Oct 12, 2023
@bertiqwerty bertiqwerty linked an issue Oct 12, 2023 that may be closed by this pull request
@jduerholt
Copy link
Contributor

Todo list:

  • review all model validators, we should switch everywhere to mode "after", this is more failsafe
  • add validators for inputs and outputs objects to check for unique keys
  • intotroduce commone type for unique str tuples

@jduerholt
Copy link
Contributor

There are some bugs in pydantic2:

  • Order of AnyFeature affects the correct deserialization
  • cannot deal with update_hyperparams as abstract method ...

Things to consider:

  • Look at order_id for features
  • make one place for custom types

@jduerholt
Copy link
Contributor

@bertiqwerty: Only failing tests are now due to failure in the DOE module since on the 25th of December a new formulaic version was released and this breaks some of our tests.

@jduerholt jduerholt marked this pull request as ready for review January 3, 2024 15:55
@jduerholt jduerholt requested review from jduerholt and removed request for jduerholt January 3, 2024 15:55
@jduerholt
Copy link
Contributor

jduerholt commented Jan 3, 2024

Hi @bertiqwerty,

tests are now passing, and I think the PR is ready for review.

What do you think, who should review it?

Issues that are mentioned above which are still open are:

  • create a central place for our custom types like bofire/data_models/types.py
  • on several places in the code we use lists that constrain strings which have to be unique (examples are the features attribute from constraints, the categories attribute from categorical features etc.). In my opinion we should switch them all to tuples and create a custom type for this.

But it could be better to do this in a seperate PR.

What do you think?

Furthermore, we need to think about a merging/release strategy as this PR introduces quite a big change/break.

Best,

Johannes

@bertiqwerty
Copy link
Contributor Author

Hi @jduerholt ,

Thanks a lot! The person for review would probably be Benny. I will also have a look. Having a central types.py makes sense to me. We should do this in a separate PR.

Copy link
Contributor Author

@bertiqwerty bertiqwerty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Johannes. I cannot approve the PR because Github says authors cannot approve their own PRs. You can probably approve and merge it?

bofire/data_models/constraints/nchoosek.py Outdated Show resolved Hide resolved
bofire/data_models/constraints/nchoosek.py Outdated Show resolved Hide resolved
bofire/data_models/strategies/predictives/botorch.py Outdated Show resolved Hide resolved
bofire/data_models/strategies/predictives/botorch.py Outdated Show resolved Hide resolved
bofire/data_models/surrogates/fully_bayesian.py Outdated Show resolved Hide resolved
bofire/surrogates/diagnostics.py Outdated Show resolved Hide resolved
@jduerholt
Copy link
Contributor

Hi @bertiqwerty,

thx! Yes, I will go through it and then approve and merge. For me it works.

The idea with the reportUnnecessaryTypeIgnoreComment is a good one! Maybe we should also switch to a more recent pyright? We are still on version 1.1.305.

Before than merging it in, should we make a last pydantic v1 release?

Best,

Johannes

@bertiqwerty
Copy link
Contributor Author

Hi Johannes,

I think we don't need another release with Pydantic 1 but I also don't have an objection. So I am fine either way.

Updating Pyright is a good idea. This often leads to some issues. So we could create a MR for this. I can look into this.

@jduerholt
Copy link
Contributor

Ok, then we do not create a Pydantic 1 release. Would be nice if you can take care for the pyright update. But we should do it after merging in this PR. My plan is to have it in the latest tomorrow. Note that I moved the pyright config some time ago into the pyproject.toml, so the new flag for showing the unnecessary ignore statements should also go there, I think:

[tool.pyright]

@jduerholt jduerholt merged commit 59bbd57 into main Jan 15, 2024
10 checks passed
@jduerholt
Copy link
Contributor

@bertiqwerty: it is now merged, you can now have a look at the pyright update ;)

@jduerholt jduerholt deleted the pydantic-2-migration branch January 15, 2024 22:56
@bertiqwerty
Copy link
Contributor Author

I am in favor of making a new release with the new Pydantic version.

@jduerholt
Copy link
Contributor

Ok, fine for me. Do you want to do it or should I do it?

@bertiqwerty
Copy link
Contributor Author

I can.

@bertiqwerty bertiqwerty mentioned this pull request Jan 17, 2024
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

Successfully merging this pull request may close these issues.

Pydantic 2
2 participants