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

openfecli tests require nagl #1129

Open
mikemhenry opened this issue Feb 14, 2025 · 8 comments
Open

openfecli tests require nagl #1129

mikemhenry opened this issue Feb 14, 2025 · 8 comments
Assignees
Labels
bug Something isn't working deployment
Milestone

Comments

@mikemhenry
Copy link
Contributor

Running the CLI tests without NAGL in your env results in a handful of tests failing: The NAGL toolkit is not available, you may be using an older version of the OpenFF toolkit - you need v0.14.4 or above We should setup our tests to skip these tests if NAGL isn't installed.

@mikemhenry mikemhenry self-assigned this Feb 14, 2025
@mikemhenry mikemhenry added bug Something isn't working deployment labels Feb 14, 2025
@mikemhenry mikemhenry added this to the v1.3.0 milestone Feb 14, 2025
@IAlibay
Copy link
Member

IAlibay commented Feb 14, 2025

I'm not too keen on a torch dependency, but how do we feel about just making Nagl-base a build requirement?

@mikemhenry
Copy link
Contributor Author

if nagl-base is enough for the tests to pass, then I don't think it is a bad idea, my fix for this was to just make the tests only run if nagl is in the env, not add nagl as a runtime dep for openfe

@IAlibay
Copy link
Member

IAlibay commented Feb 14, 2025

Yeah I think we only need Nagl-base. @jthorton might know better here.

@mikemhenry
Copy link
Contributor Author

@IAlibay
Copy link
Member

IAlibay commented Feb 14, 2025

:/ I don't know then.

The view from "up top" is that NAGL is going to become a common place tool in our pipelines, iirc the next OpenFF release is going to be parameterised using NAGL. That being said, I don't have (or want to have) any strong takes at what this means at the package level. If dependency size is a big deal, we should report it upstream, it's a legit concern openfe partners have raised.

@mikemhenry
Copy link
Contributor Author

Well for now we can just skip running the tests in the openfecli like we do in openfe if the package doesn't exist, I would like to create gufe-base and openfe-base packages that don't have things like charge backends and md engines anyway

@mikemhenry mikemhenry mentioned this issue Feb 14, 2025
2 tasks
@jthorton
Copy link
Collaborator

Ah yes we should have added the skip if not installed in the first place while NAGL is still considered optional. I could also mock the charge generation if we don't want most tests to use nagl this was just to keep the CI quick but mocking would work here as well!

@mikemhenry
Copy link
Contributor Author

I think we are okay, since if a user installs nagl then the tests will run, and we have nagl installed on our CI env.

We might want to update the install instructions to say "also install whatever charge backend you are going to use before you run the tests so that it gets tested"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working deployment
Projects
None yet
Development

No branches or pull requests

3 participants