search

openforcefield / openff-models

Helper classes for Pydantic compatibility in the OpenFF stack
MIT License
3 stars 2 forks source link

Use `BaseModel` instead of `DefaultModel` #5

Open mikemhenry opened 2 years ago

mattwthompson commented 2 years ago

I think we need DefaultModel to get the custom JSON encoder and decoder into the subclassed models? https://github.com/mattwthompson/openff-models/blob/61260ec1d2746a0431671498dfe1540b006354db/openff/models/models.py#L13-L16

mikemhenry commented 2 years ago

For that, I think we can do pydantic.BaseModel and still call ours BaseModel

mikemhenry commented 2 years ago

Sorry I wrote this issue as a quick thing and didn't expect you to look at it, the tl;dr is that we were thinking it would be easier to just use the same name as pydantic, then people could drop ours in/copy and paste an example in pydantic and it would JustWork :tm:

mattwthompson commented 2 years ago

Maybe it's just a matter of preference and naming - overloading BaseModel make me nervous since it's hard to be sure which one I have imported. The whole motivation of creating DefaultModel is to create a slight variant of BaseModel that does what I want, but is denoted explicitly as a different thing.

mikemhenry commented 2 years ago

That is a good argument. Perhaps then something even more explicit like OFFBaseModel? I think @richardjgowers and @dwhswenson had some opinions on this as well (this is also something really easy to change so perhaps we don't worry about it until we are ready for a more public release).

dwhswenson commented 2 years ago

Is a user likely to have both pydantic.BaseModel and openff.models.DefaultModel imported? If I'm using OpenFF units in my project, I assume I'll make all my models inherit from this one -- is there anything pydantic does that can't be done with this one?

It also means that when a user adds OpenFF unit support to a project with existing pydantic models, switching is just s/pydantic/openff.models/.

mattwthompson commented 2 years ago

Perhaps then something even more explicit like OFFBaseModel?

I'd be fine with something like this - DefaultModel is arbitrary and not a name I put much effort into. I'd personally prefer something like OpenFFBaseModel or OpenFFModel for short.

Is a user likely to have both pydantic.BaseModel and openff.models.DefaultModel imported?

Most users probably won't, and it probably doesn't make much sense for simple uses cases i.e. what we'd write examples for. But people want to do creative things with our tools and I don't want to assume they wouldn't mix something from OpenFF/OpenFE/OpenMM and FastAPI or some other framework that's likely to assume pydantic.BaseModel is there. So my preference is for keeping the names separate and their origins a bit more explicit.

switching is just

The (adversarial, I'l admit!) counterpoint for situations in which users might want to mix where BaseModel comes from would require something like

from pydantic import BaseModel
from openff.models import BaseModel as OpenFFBaseModel

which could just be done from the beginning.