openff-evaluator icon indicating copy to clipboard operation
openff-evaluator copied to clipboard

Specified skip_on_failure=True

Open jaketanderson opened this issue 1 year ago • 5 comments

To make paprika_testing work with pydantic version 2.xx, the root_validator decorator needs to specify skip_on_failure=True.

Todos

Notable points that this PR has either accomplished or will accomplish.

  • [X] Specified skip_on_failure=True for all instances of root_validator decorator in datasets/curation/filtering.py.

Status

  • [X] Ready to go

jaketanderson avatar Jul 06 '24 05:07 jaketanderson

Is this the only change needed to get the codebase working with v2?

I'd prefer using the v1 API from v2, just to be explicit in which is actually being used, like so:

try:
   # use v1 from v2 ...
    from pydantic.v1 import root_validator
except ImportError:
    # ... unless we have v1 installed, then just use it
    from pydantic import root_validator

mattwthompson avatar Jul 08 '24 15:07 mattwthompson

Is this the only change needed to get the codebase working with v2?

I haven't done extensive testing, but in my ordinary use of paprika_testing with v2 I haven't encountered any further pydantic issues.

Your suggestion is much better, I didn't realize v1 was nicely exposed in v2. Upon further inspection it looks like the proper way to do things for pydantic v2 is to use exclusively @field_validator():

[...]openff-evaluator/openff/evaluator/datasets/curation/components/filtering.py:1045: PydanticDeprecatedSince20:
Pydantic V1 style `@validator` validators are deprecated.
You should migrate to Pydantic V2 style `@field_validator` validators, see the migration guide for more details.
Deprecated in Pydantic V2.0 to be removed in V3.0. See Pydantic V2 Migration Guide at
https://errors.pydantic.dev/2.8/migration/

Although this would be the proper way to do things, I think it's unnecessary since I'm probably the only one (or one of a very small few) actively using this branch. I think it'd be adequate to use the method in your comment and restrict the version of pydantic to <3 in devtools/conda-envs/test_env.yaml. What are your thoughts?

jaketanderson avatar Jul 12 '24 19:07 jaketanderson

What's going on with paprika_testing and what are the plans? Do you wish for it to be in a released version of Evaluator?

mattwthompson avatar Jul 15 '24 15:07 mattwthompson

@mattwthompson sorry for the lack of timely responses, I've recently been committed to serve on a jury and that has slowed down my projects (which use Evaluator). I'm really not sure what the future of this branch is. I think I'll ping @jeff231li and see if he has any intention or need for it to be supported in the future. My guess is that there are still things we could/should try to add to another branch, whether it be main or paprika-latest-features, but I don't know how mature those paprika things are. I'm not sure if it's a goal to have paprika's protocol implemented in Evaluator the way Yank's protocol is (or was) implemented for hydrations, do you have an idea of what people would like to see? Should there be a mature paprika protocol as a go-to option for evaluating host guest binding energies?

jaketanderson avatar Jul 19 '24 23:07 jaketanderson

Nothing to apologize for - I'm here to support scientists like yourself, so I'm not harmed by unlucky slowdowns like jury duty 🙂

I'm asking around to see if this is a priority for OpenFF; in general our focus is keeping the latest release of each package online, so when long-term development happens in non-main branches it's easy for me to lose track of what's going on.

mattwthompson avatar Jul 22 '24 17:07 mattwthompson