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

Register solvation-analysis as MDAKit #67

Merged
merged 5 commits into from
Sep 19, 2023

Conversation

orionarcher
Copy link
Contributor

Thanks for the straightfoward tutorial, this was a painless process.

My one question is regarding the run_tests command. Where should this command be executed from? I've written it to be executed from package root. I am a bit confused by the pytest --pyargs propkatraj.tests syntax in the example.

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good, just make pytest find the tests.

mdanalysis_requires: ">=2.0.0"
## List(str): a list of commands to use when attempting to run the MDAKit's tests
run_tests:
- pytest solvation_analysis/tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not necessarily have your checked-out sources available when the package was conda- or pip-installed. Given that you have the tests included in your package as solvation_analysis.tests try and have pytest find them using --pyargs:

pytest -n auto --pyargs solvation_analysis.tests

(There are shortcomings to using --pyargs in specific cases that @IAlibay knows more about but let's try it for right now. We use it for a bunch of other mdakits.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you package your tests then yes, here you should try to use --pyargs, the issue with using pyargs is mostly limited to if you're using custom plugins, but those are usually for internal testing use.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it to work and changed in the commit, thanks y'all.

@orbeckst orbeckst self-assigned this Sep 13, 2023
mdakits/solvation-analysis/metadata.yaml Show resolved Hide resolved
mdakits/solvation-analysis/metadata.yaml Outdated Show resolved Hide resolved
mdakits/solvation-analysis/metadata.yaml Outdated Show resolved Hide resolved
Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests still failing because of pytest issues but should be a simple fix: either include pytest-xdist or remove -n auto.

mdakits/solvation-analysis/metadata.yaml Outdated Show resolved Hide resolved
## release of the code. Note: only one installation method can currently
## be defined. We suggest using conda/mamba where possible.
install:
- mamba install -c conda-forge solvation_analysis "pandas<2"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See: MDAnalysis/solvation-analysis#93

Ideally if you can either pin your recipees or fix the bug & do a release that would be great, we probably can't have the pandas pin in the install tagline.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orionarcher please review

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the other PRs have been addressed, we remove the pin from here

Suggested change
- mamba install -c conda-forge solvation_analysis "pandas<2"
- mamba install -c conda-forge solvation_analysis

I hope that the conda-forge PR is reviewed in time so that cf can build an updated package before our Thu 9/21 deadline.

@orbeckst
Copy link
Member

@orionarcher we'd really like to get solvation-analysis into the registry before the UGM as it would make for an excellent example. For this to work, we have a merge deadline of Thursday 9/21. Would you be able to make some time to have a look at the relevant PRs mentioned above and work with us to get your kit registered?

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming CI passes, lgtm!

@IAlibay
Copy link
Member

IAlibay commented Sep 19, 2023

@orbeckst Since everything has now been addressed I'm going to discard your review and merge (that way we can quickly tell if anything will fall apart on deployment).

@IAlibay IAlibay merged commit 23ebc75 into MDAnalysis:main Sep 19, 2023
3 checks passed
Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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.

3 participants