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

Specified skip_on_failure=True #558

Open
wants to merge 1 commit into
base: paprika_testing
Choose a base branch
from

Conversation

jaketanderson
Copy link
Contributor

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.

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

Status

  • Ready to go

To make paprika_testing work with pydantic version 2.xx, the `root_validator` decorator needs to specify skip_on_failure=True.
@mattwthompson
Copy link
Member

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

@jaketanderson
Copy link
Contributor Author

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?

@mattwthompson
Copy link
Member

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?

@jaketanderson
Copy link
Contributor Author

@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?

@mattwthompson
Copy link
Member

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.

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.

2 participants