search

openclimatefix / nowcasting_datamodel

Datamodel for the nowcasting project
6 stars 6 forks source link

from_orm method is deprecated in pydantic V2 #284

Closed Bvr4 closed 2 months ago

Bvr4 commented 5 months ago

While working on the issue #281, I saw that the from_orm method from pydantic V1 is deprecated in pydantic V2. This method is still present in some models.

It would be interesting as well to verify if other deprecated pydantic methods are still presents in the models.

Bvr4 commented 5 months ago

I would like to work on this issue, can it be assigned to me ?

peterdudfield commented 5 months ago

I would like to work on this issue, can it be assigned to me ?

of course - thanks @Bvr4

peterdudfield commented 5 months ago

It might be worth checking https://github.com/openclimatefix/pv-site-datamodel this repo, to see if any pydantic upgrades need to happen

Bvr4 commented 5 months ago

I did a quick search on this particular from_orm method, it is used in several repos (nowcasting-metrics, ocf_datapipes, uk-analysis-dashboard, GSPConsumer...) I'll do a detailed check on pv-site-datamodel later.

Bvr4 commented 5 months ago

I have a question relative to this issue. The pydantic migration guide advises us to use model_validate instead of the deprecated from_orm method. In the Forecast model, there is a from_orm_latest method. Would it be judicious to rename it model_validate_latest ? I think it would be more consistent.

peterdudfield commented 5 months ago

Just to check, does model_validate change from sql to pydantic? I think that what from_orm did?

Bvr4 commented 5 months ago

Yes, model_validate transforms any object (in this case, an ORM instance from SQL) into a pydantic object, after validations. It works in the same way as from_orm or parse_obj in V1. I don't know why they deprecated from_orm though.

Bvr4 commented 5 months ago

The Forecast and ForecastValue models overwrites pydantic from_orm method. We can change that so they overwrites the model_validate method. But if from_orm method is called by other projects, we might face unexpected behaviors (as it seams from_orm can still be called with pydantic V2, but raises DeprecationWarnings). Maybe we could keep overwrited from_orm method aside a new overwrited model_validate method ? So we won't face problems if other projects still uses from_orm for this models.

peterdudfield commented 5 months ago

The Forecast and ForecastValue models overwrites pydantic from_orm method. We can change that so they overwrites the model_validate method. But if from_orm method is called by other projects, we might face unexpected behaviors (as it seams from_orm can still be called with pydantic V2, but raises DeprecationWarnings). Maybe we could keep overwrited from_orm method aside a new overwrited model_validate method ? So we won't face problems if other projects still uses from_orm for this models.

Yea, thats a good idea. Yea perhaps just this line could be updated, but keep that function

Bvr4 commented 5 months ago

It might be worth checking https://github.com/openclimatefix/pv-site-datamodel this repo, to see if any pydantic upgrades need to happen

The use of pydantic is quite limited in this repo, there's no upgrade needed to match pydantic V2 recommendations.