Add MD-SAPT to MDAKits #47

ALescoulie · 2023-04-17T20:16:35Z

My MDAKit MD-SAPT runs SAPT calculations over MD trajectories using Psi4. We currently are working on publishing it in JOSS and it is ready to be submitted as an official MDA kit.

IAlibay · 2023-04-17T20:20:01Z

@ALescoulie we're in the process of improving our docs for adding mdakits, especially when it comes to adding things so that tests will run properly. We'll come back to this (and other PRs open) once that's done, so that we can point folks to the right steps to getting everything working.

orbeckst · 2023-07-03T01:33:30Z

FYI, we have the MDAKits tutorial with the section Submitting a MDAKit to the registry.

orbeckst

Meta data requires some obvious changes, in particular the ones to make tests run.

mdakits/mdsapt/metadata.yaml

orbeckst · 2023-07-03T01:36:27Z

mdakits/mdsapt/metadata.yaml

+## str: the development status of the MDAKit
+development_status: Mature
+## List(str) a list of publications to cite when using the MDAKit
+publications:


paper published?

not yet, were working on it

orbeckst · 2023-07-03T01:38:06Z

mdakits/mdsapt/metadata.yaml

+development_status: Mature
+## List(str) a list of publications to cite when using the MDAKit
+publications:
+community_home:


Maybe enable GitHub discussions on https://github.com/calpolyccg/MDSAPT ??

I enabled discussions

Can you add an link to discussions?

mdakits/mdsapt/metadata.yaml

orbeckst · 2023-09-12T19:32:16Z

@ALescoulie could you address the changes above? They are really minor!

mdakits/mdsapt/metadata.yaml

- conda package requires dev label - install pre-requisites for source installation

orbeckst · 2023-09-18T15:25:06Z

I played around with the config file to see if I could make it pass the MDAKit tests.

Note that psi4 migrated from their psi4 channel to conda-forge since 1.8.1. However, MDSAPT is pinning psi4>=1.6.1,<1.7 so the psi4 channel is required. Also, mdsapt only exists as a dev package in the psi4 channel at the moment.

orbeckst · 2023-09-18T15:32:17Z

The tests don't like to be run with --pyargs

Run tests=$(python utils/get_runtests.py --mdakit mdsapt --runtype latest)
tests: pytest --pyargs mdsapt.tests
ERROR: module or package not found: mdsapt.tests (missing __init__.py?)

Not sure if mdsapt is not installed properly or if it's something else; the tests directory contains __init__.py https://github.com/calpolyccg/MDSAPT/tree/master/mdsapt/tests.

orbeckst · 2023-09-18T15:45:20Z

The tests fail because some issues with pydantic. This looks as if it needs to be fixed in MD-SAPT

==================================== ERRORS ====================================
_________________ ERROR collecting mdsapt/tests/test_config.py _________________
/usr/share/miniconda3/envs/test/lib/python3.9/site-packages/_pytest/runner.py:341: in from_call
    result: Optional[TResult] = func()
/usr/share/miniconda3/envs/test/lib/python3.9/site-packages/_pytest/runner.py:372: in <lambda>
    call = CallInfo.from_call(lambda: list(collector.collect()), "collect")
/usr/share/miniconda3/envs/test/lib/python3.9/site-packages/_pytest/python.py:531: in collect
    self._inject_setup_module_fixture()
/usr/share/miniconda3/envs/test/lib/python3.9/site-packages/_pytest/python.py:545: in _inject_setup_module_fixture
    self.obj, ("setUpModule", "setup_module")
/usr/share/miniconda3/envs/test/lib/python3.9/site-packages/_pytest/python.py:310: in obj
    self._obj = obj = self._getobj()
/usr/share/miniconda3/envs/test/lib/python3.9/site-packages/_pytest/python.py:528: in _getobj
    return self._importtestmodule()
/usr/share/miniconda3/envs/test/lib/python3.9/site-packages/_pytest/python.py:617: in _importtestmodule
    mod = import_path(self.path, mode=importmode, root=self.config.rootpath)
/usr/share/miniconda3/envs/test/lib/python3.9/site-packages/_pytest/pathlib.py:567: in import_path
    importlib.import_module(module_name)
/usr/share/miniconda3/envs/test/lib/python3.9/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
<frozen importlib._bootstrap>:1030: in _gcd_import
    ???
<frozen importlib._bootstrap>:1007: in _find_and_load
    ???
<frozen importlib._bootstrap>:972: in _find_and_load_unlocked
    ???
<frozen importlib._bootstrap>:228: in _call_with_frames_removed
    ???
<frozen importlib._bootstrap>:1030: in _gcd_import
    ???
<frozen importlib._bootstrap>:1007: in _find_and_load
    ???
<frozen importlib._bootstrap>:972: in _find_and_load_unlocked
    ???
<frozen importlib._bootstrap>:228: in _call_with_frames_removed
    ???
<frozen importlib._bootstrap>:1030: in _gcd_import
    ???
<frozen importlib._bootstrap>:1007: in _find_and_load
    ???
<frozen importlib._bootstrap>:986: in _find_and_load_unlocked
    ???
<frozen importlib._bootstrap>:680: in _load_unlocked
    ???
<frozen importlib._bootstrap_external>:850: in exec_module
    ???
<frozen importlib._bootstrap>:228: in _call_with_frames_removed
    ???
mdsapt/__init__.py:8: in <module>
    from .config import Config, load_from_yaml_file
mdsapt/config.py:132: in <module>
    class RangeFrameSelection(BaseModel):
mdsapt/config.py:145: in RangeFrameSelection
    @root_validator()
/usr/share/miniconda3/envs/test/lib/python3.9/site-packages/pydantic/deprecated/class_validators.py:228: in root_validator
    raise PydanticUserError(
E   pydantic.errors.PydanticUserError: If you use `@root_validator` with pre=False (the default) you MUST specify `skip_on_failure=True`. Note that `@root_validator` is deprecated and should be replaced with `@model_validator`.
E   
E   For further information visit https://errors.pydantic.dev/2.3/u/root-validator-pre-skip

orbeckst

There are problems with pydantic that makes the tests fail, at least with the versions that are used in the testing here. I raised calpolyccg/MDSAPT#42 . When the testing code is changed, we can re-run here.

Please ping me.

orbeckst · 2023-09-18T15:55:56Z

mdakits/mdsapt/metadata.yaml

+## source code.
+src_install:
+  # required dependencies from environment.yaml
+  - mamba install -c psi4 -c conda-forge -c defaults "psi4>=1.6.1,<1.7" click numpy openmm pandas pdbfixer pytest pydantic pytest-cov pyyaml rdkit


Question for @IAlibay : Is it correct to do the installation of dependencies before the source pip install if I don't want dependencies to be pip-installed? I'd love to be able to say mamba install -f environment.yml but to my knowledge that doesn't work (only works with mamba create). Because I have to list dependencies explicitly, I also have to explicitly add pinning (like for psi4). That looks a bit awkward and prone to breakage. Is there a better way?

🤔 yes that is indeed a bit of a problem.

What would the desired workflow be here? Fetch the yaml file from the remote directory and create the environment using that? Or should we try to read the yaml file and install from there?

This is definitely something to be considered as part of the refactor.

At the end of the day the package developers should be responsible for keeping the meta data in sync with the package installation. I’d rather keep our side simple and suggest that ultimately packages follow standard procedures. In the meantime explicit pinning should be fine as a temporary measure — if this gets the kit installed correctly.

ALescoulie · 2024-06-24T03:42:50Z

we now have a conda-forge release (conda install -c conda-forge mdsapt, does that mean I have to still list required dependencies in src_install or can I let conda-forge take care of that?

ALescoulie · 2024-06-24T03:47:51Z

I pushed an update that has the conda forge install, I'd appreciate if someone could take a look at this.

IAlibay

Sorry just giving this a very brief look, the src_install field expects a list, added a suggested change to fix that.

mdakits/mdsapt/metadata.yaml

Co-authored-by: Irfan Alibay <[email protected]>

ALescoulie · 2024-06-24T20:07:24Z

Icommitted @IAlibay's change to dependency install command, and fixed an issue with the test dependencies command.

@IAlibay and @orbeckst is there anything else to fix for this PR?

ALescoulie · 2024-07-02T18:31:02Z

The CI failed because I put a comma between test dependencies on the mamba install line which caused it to fail. I pushed a fix, @orbeckst could you try running the full CI again?

orbeckst · 2024-07-02T19:54:54Z

There was a fail for develop and I restarted it. Not sure if this is what you meant.

orbeckst

The develop CI is still failing https://github.com/MDAnalysis/MDAKits/actions/runs/9766164949/job/26961722361?pr=47

Does the command

mamba install -c conda-forge psi4 >= 1.9.1, mdanalysis >=2.7.0, rdkit >=2023.09.5, openmm >=8.1.1, pdbfixer >=1.9, numpy, click, pandas, pyarrow, pyyaml;pip install git+https://github.com/calpolyccg/MDSAPT@master

work when you try it manually?

orbeckst · 2024-07-02T19:58:39Z

(Is it possible that there should be quotation marks in the install command?)

IAlibay

I think this should work

mdakits/mdsapt/metadata.yaml

Co-authored-by: Irfan Alibay <[email protected]>

orbeckst

Develop installation works but you get a segfault

Thread 0x00007f8ad06fe640/home/runner/work/_temp/e522ce51-b634-481c-b233-3008ac944e02.sh: line 3:  3289 Segmentation fault      (core dumped) pytest -v ./mdsapt/tests
mdsapt/tests/test_sapt.py::TestSAPT::test_run_traj_sapt

Can you look into what's happening there? Is this locally reproducible?

ALescoulie · 2024-07-03T14:34:04Z

Yeah I'll look at that later day. The issue probably is that need to bring develop up to date with master because we've pushed a lot of the recent fixes straight from feature branch into master.

orbeckst

Fix segfault in develop CI.

(Excuse brevity, please)

orbeckst · 2024-07-03T15:07:29Z

mdakits/mdsapt/metadata.yaml

+development_status: Mature
+## List(str) a list of publications to cite when using the MDAKit
+publications:
+community_home:


Can you add an link to discussions?

orbeckst · 2024-07-03T15:11:37Z

mdakits/mdsapt/metadata.yaml

+src_install:
+  # required dependencies from environment.yaml
+  - mamba install -c conda-forge "psi4>= 1.9.1" "mdanalysis >=2.7.0" "rdkit >=2023.09.5" "openmm >=8.1.1" "pdbfixer >=1.9" numpy click pandas pyarrow pyyaml
+  - pip install git+https://github.com/calpolyccg/MDSAPT@master


Note that the „develop“ CI installs your source (ie your master branch) and tests against the MDAnalysis develop branch! That’s the configuration to check for the segfault.

orbeckst · 2024-08-15T05:22:22Z

Have you made any progress in figuring out the reason for the segfault? Is it even locally reproducible?

ALescoulie · 2024-08-16T22:45:53Z

@orbeckst I haven't gotten around to trying yet, I forgot to mention on this thread that I just moved (I'm still in the bay area, my partner and I found a great deal on a place in SF, so jumped on it) and have been busy with that. Now that I finally set up my desktop again, I'll try and replicate it in a container since I use NixOS and don't want to potentially introduce Nix related problems to the process.

orbeckst · 2024-09-12T19:53:42Z

Ping @ALescoulie ;-) — did you have some opportunities to dig into MD-SAPT again?

Create metadata.yaml

e402a16

orbeckst requested changes Jul 3, 2023

View reviewed changes

Update metadata.yaml

ac07c21

orbeckst reviewed Sep 18, 2023

View reviewed changes

mdakits/mdsapt/metadata.yaml Outdated Show resolved Hide resolved

orbeckst reviewed Sep 18, 2023

View reviewed changes

mdakits/mdsapt/metadata.yaml Show resolved Hide resolved

install mdsapt properly

4f2f9ed

- conda package requires dev label - install pre-requisites for source installation

get tests from checked out source

1a67667

orbeckst mentioned this pull request Sep 18, 2023

update/fix pydantic for testing calpolyccg/MDSAPT#42

Closed

orbeckst requested changes Sep 18, 2023

View reviewed changes

orbeckst reviewed Sep 18, 2023

View reviewed changes

Merge branch 'MDAnalysis:main' into main

7dd677c

add conda-forge install

f6f007f

IAlibay reviewed Jun 24, 2024

View reviewed changes

mdakits/mdsapt/metadata.yaml Outdated Show resolved Hide resolved

ALescoulie and others added 2 commits June 24, 2024 12:35

make src-install into a list

d30ea9d

Co-authored-by: Irfan Alibay <[email protected]>

fix test dependencies

2001829

ALescoulie requested a review from orbeckst June 24, 2024 20:05

IAlibay assigned orbeckst Jul 1, 2024

fix comma typo in test deps install

a01a628

orbeckst requested changes Jul 2, 2024

View reviewed changes

IAlibay reviewed Jul 2, 2024

View reviewed changes

mdakits/mdsapt/metadata.yaml Outdated Show resolved Hide resolved

Update mdakits/mdsapt/metadata.yaml

840e2a5

Co-authored-by: Irfan Alibay <[email protected]>

orbeckst requested changes Jul 2, 2024

View reviewed changes

orbeckst requested changes Jul 3, 2024

View reviewed changes

orbeckst added the new create a new MDAKit label Aug 1, 2024

orbeckst added the mdakit About an MDAKit. label Sep 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MD-SAPT to MDAKits #47

Add MD-SAPT to MDAKits #47

ALescoulie commented Apr 17, 2023

IAlibay commented Apr 17, 2023

orbeckst commented Jul 3, 2023

orbeckst left a comment

orbeckst Jul 3, 2023

ALescoulie Jun 25, 2024

orbeckst Jul 3, 2023

ALescoulie Jun 25, 2024

orbeckst Jul 3, 2024

orbeckst commented Sep 12, 2023

orbeckst commented Sep 18, 2023

orbeckst commented Sep 18, 2023

orbeckst commented Sep 18, 2023

orbeckst left a comment •

edited

Loading

orbeckst Sep 18, 2023

IAlibay Dec 24, 2023

orbeckst Dec 24, 2023

ALescoulie commented Jun 24, 2024

ALescoulie commented Jun 24, 2024 •

edited

Loading

IAlibay left a comment

ALescoulie commented Jun 24, 2024

ALescoulie commented Jul 2, 2024

orbeckst commented Jul 2, 2024

orbeckst left a comment

orbeckst commented Jul 2, 2024

IAlibay left a comment

orbeckst left a comment

ALescoulie commented Jul 3, 2024

orbeckst left a comment

orbeckst Jul 3, 2024

orbeckst Jul 3, 2024

orbeckst commented Aug 15, 2024

ALescoulie commented Aug 16, 2024

orbeckst commented Sep 12, 2024

Add MD-SAPT to MDAKits #47

Are you sure you want to change the base?

Add MD-SAPT to MDAKits #47

Conversation

ALescoulie commented Apr 17, 2023

IAlibay commented Apr 17, 2023

orbeckst commented Jul 3, 2023

orbeckst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbeckst commented Sep 12, 2023

orbeckst commented Sep 18, 2023

orbeckst commented Sep 18, 2023

orbeckst commented Sep 18, 2023

orbeckst left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ALescoulie commented Jun 24, 2024

ALescoulie commented Jun 24, 2024 • edited Loading

IAlibay left a comment

Choose a reason for hiding this comment

ALescoulie commented Jun 24, 2024

ALescoulie commented Jul 2, 2024

orbeckst commented Jul 2, 2024

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst commented Jul 2, 2024

IAlibay left a comment

Choose a reason for hiding this comment

orbeckst left a comment

Choose a reason for hiding this comment

ALescoulie commented Jul 3, 2024

orbeckst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbeckst commented Aug 15, 2024

ALescoulie commented Aug 16, 2024

orbeckst commented Sep 12, 2024

orbeckst left a comment •

edited

Loading

ALescoulie commented Jun 24, 2024 •

edited

Loading