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

Add openmmdl as MDAKit #161

Merged
merged 30 commits into from
Sep 21, 2024
Merged

Add openmmdl as MDAKit #161

merged 30 commits into from
Sep 21, 2024

Conversation

talagayev
Copy link
Member

Addition of OpenMMDL to MDAKits:

Package for Interface to OpenMM for easy setup of molecular dynamic simulations of protein-ligand complexes and the analysis thereof, available at:

https://anaconda.org/conda-forge/openmmdl

Created metadata file for openmmdl
@orbeckst orbeckst added the new create a new MDAKit label Aug 28, 2024
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.

The main issue is that one set of tests for py 3.12 fails with an installation issue for mdtraj


Could not solve for environment specs
The following package could not be installed
└─ openmmdl is installable and it requires
   └─ mdtraj 1.9.7.*  with the potential options
      ├─ mdtraj 1.9.7 would require
      │  └─ python >=3.10,<3.11.0a0 , which can be installed;
      ├─ mdtraj 1.9.7 would require
      │  └─ python >=3.11,<3.12.0a0 , which can be installed;
      ├─ mdtraj 1.9.7 would require
      │  └─ python >=3.7,<3.8.0a0 , which can be installed;
      ├─ mdtraj 1.9.7 would require
      │  └─ python >=3.8,<3.9.0a0 , which can be installed;
      └─ mdtraj 1.9.7 would require
         └─ python >=3.9,<3.10.0a0 , which can be installed.

I am not really sure what's going on so I am bouncing this back to you to look into and report back...

UPDATE: Looking more closely into the "develop" test run, which supposedly passed, I can now see that this run did not actually install the package:


Run if [[ develop == "develop" ]]; then
  if [[ develop == "develop" ]]; then
    type="src"
  else
    type="install"
  fi
  
  install=$(python utils/get_install.py --itype ${type} --mdakit openmmdl)
  echo "install=${install}"
  eval ${install}
  shell: /usr/bin/bash -l {0}
  env:
    PYVER: 
    INPUT_RUN_POST: true
    CONDA: /usr/share/miniconda3
    CONDA_PKGS_DIR: /home/runner/conda_pkgs_dir
install=

Similarly, the "run tests" also come back empty and report success

Run tests=$(python utils/get_runtests.py --mdakit openmmdl --runtype develop)
  tests=$(python utils/get_runtests.py --mdakit openmmdl --runtype develop)
  echo "tests: ${tests}"
  eval ${tests}
  shell: /usr/bin/bash -l {0}
  env:
    PYVER: 
    INPUT_RUN_POST: true
    CONDA: /usr/share/miniconda3
    CONDA_PKGS_DIR: /home/runner/conda_pkgs_dir
tests: 

This means that you also need instructions how to install from source (and my hunch is that the testing command that I already mentioned also needs updating).

mdakits/openmmdl/metadata.yaml Outdated Show resolved Hide resolved
mdakits/openmmdl/metadata.yaml Show resolved Hide resolved
@talagayev
Copy link
Member Author

@orbeckst hey Oliver,

Hm I can take a look into mdtraj to adjust the mdtraj version, since the code mainly uses mdtraj only for one function, which ist image_molecules(), else I can just replace it with an mdanalysis function to do the same :)

@talagayev
Copy link
Member Author

Ah ambertools also complains about the imcompatability with the python version, so that currently only python 3.10 is the python version covering all of the packages. I will then update the packages and test them to be compatible with python 3.11 and python 3.12, make a release and get then the compatable release version on conda-forge so that it works with all 3 python versions.

@IAlibay
Copy link
Member

IAlibay commented Sep 1, 2024

@lilyminium @fiona-naughton can either of you assign yourselves to shepherd this PR?

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.

✅ The openmmmdl (latest) installs and runs the tests successfully.
❌ The openmmmdl (develop) does not properly install, see e.g. https://github.com/MDAnalysis/MDAKits/actions/runs/10688427502/job/29628268563?pr=161#step:7:18 — the following steps all complete seemingly successfully

The develop tests should also run properly with an installed version of the software.

@talagayev
Copy link
Member Author

@orbeckst If I understand correctly the tests are successfull with the latest version of mdanalysis, but errors appear with the devlopment version of mdanalysis, so I can check if they run locally by using the development mdanalysis package locally and looking if it installs and runs the tests successuflly?

@orbeckst
Copy link
Member

orbeckst commented Sep 3, 2024

When you look at the GH actions report at the line that I linked, you see that

install=$(python utils/get_install.py --itype ${type} --mdakit openmmdl)

gives an empty install variable.

I would assume that this is because there's an issue with the metadata.yml file and utils/get_install.py is getting confused somehow. That's where I would start.

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.

Apologies folks, I haven't been keeping up with this.

Looks like the develop failure is because there's a missing src_install entry, which is a requirement for mdakit registration.

@talagayev
Copy link
Member Author

@IAlibay hm shouldn't be a problem to add the src_install entry, the only problem is that I would not be able to have all of the packages being installable with pip due to using plip for protein-ligand interaction analysis, which uses openbabel that has the problem with the wheel building, so the installation with pip would miss this package. I will then add the src_install entry now :)

Added src_install
@talagayev
Copy link
Member Author

I added the src_install entry and will then move the packages that are possible to be installed with pip tomorrow to the pyproject.toml in the OpenMMDL repository, with the exception of plip, thanks @IAlibay for helping with the PR:)

@IAlibay
Copy link
Member

IAlibay commented Sep 3, 2024

@IAlibay hm shouldn't be a problem to add the src_install entry, the only problem is that I would not be able to have all of the packages being installable with pip due to using plip for protein-ligand interaction analysis, which uses openbabel that has the problem with the wheel building, so the installation with pip would miss this package. I will then add the src_install entry now :)

You can have a separate mamba install call before the pip install, i.e. it's a list so you can just add more things as necessary - I believe a few other mdakits do this (I'll try to find an example later if I get a chance)

@talagayev
Copy link
Member Author

@IAlibay hm shouldn't be a problem to add the src_install entry, the only problem is that I would not be able to have all of the packages being installable with pip due to using plip for protein-ligand interaction analysis, which uses openbabel that has the problem with the wheel building, so the installation with pip would miss this package. I will then add the src_install entry now :)

You can have a separate mamba install call before the pip install, i.e. it's a list so you can just add more things as necessary - I believe a few other mdakits do this (I'll try to find an example later if I get a chance)

Ah, then it should be fine, I will then go to sleep now and take a look tomorrow, since if it is possible to first run the mamba install then it shouldn't be a problem :)

adjustment of src_install
@talagayev
Copy link
Member Author

@IAlibay okay mdahole2, lipyds and openmm-mdanalysis-reporter all do it the way you mentioned, by git cloning the repository, then installing through mamba install and then going into the cloned folder and then using pip install . I will see if it works now with mamba install and directly afterwards pip install from github without the git cloning and cd into the folder

@orbeckst orbeckst self-assigned this Sep 6, 2024
@orbeckst
Copy link
Member

orbeckst commented Sep 6, 2024

@talagayev please ping me when you've made progress and think it's ready (i.e., everything installs and passes tests) or if there are questions.

@orbeckst orbeckst added the mdakit About an MDAKit. label Sep 6, 2024
@talagayev
Copy link
Member Author

@orbeckst I think everything is ready and should be now running correctly including the installation with src_install and all the pytests :)

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.

I need some clarification on what happens in the CI develop during mdakit installation:

Successfully built openmmdl
Installing collected packages: openmmdl
  Attempting uninstall: openmmdl
    Found existing installation: openmmdl 0.0.1
    Uninstalling openmmdl-0.0.1:
      Successfully uninstalled openmmdl-0.0.1
Successfully installed openmmdl-0.9.2.1+3.g2217545.dirty

The conda-forge package is at version 0.9.2.1

+ openmmdl                              0.9.2.1  pyhd8ed1ab_0                       conda-forge      85MB

and it is being installed to get all the deps.

Then the pip package is built and installed. But why does pip uninstall version 0.0.1 ??

As mentioned in my comments, it would be cleaner to explicitly list dependencies for a source installation.

mdakits/openmmdl/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.

Please see comments!

(I wrote these days ago but apparently forgot to submit review, sorry for the delay.)

mdakits/openmmdl/metadata.yaml Outdated Show resolved Hide resolved
openmmforcefields
cudatoolkit>=11.7.0
cuda-python>=11.7.1
mdanalysis>=2.3.0
Copy link
Member

Choose a reason for hiding this comment

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

mdanalysis may not be necessary because I think we install it ourselves — but I doubt it hurts.

Do you have any specific recommendations @IAlibay @fiona-naughton @lilyminium ?

Copy link
Member Author

Choose a reason for hiding this comment

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

also a similar case is for versioningit and setuptools which I have in the pyproject.toml so they are installed there in theory, but I thought if I list the remaining packages I can also add them in the mamba install step so that all of the packages necessary for the package are present

Copy link
Member Author

Choose a reason for hiding this comment

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

I will keep for now the mdanalysis>=2.3.0 to see what the others say if it should be removed or retained :)

Copy link
Member

Choose a reason for hiding this comment

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

ok

Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to have MDAnalysis here, in the end this will go in the docs as "this is how you install this".

I have moved pytest things into test_dependencies though, because as far as I'm aware they aren't necessary for the standard operations of the code.

mdakits/openmmdl/metadata.yaml Outdated Show resolved Hide resolved
@talagayev
Copy link
Member Author

Please see comments!

(I wrote these days ago but apparently forgot to submit review, sorry for the delay.)

will do, all good :)

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.

Thank you for addressing all my comments. Looks ready to me (modulo comments from others).

openmmforcefields
cudatoolkit>=11.7.0
cuda-python>=11.7.1
mdanalysis>=2.3.0
Copy link
Member

Choose a reason for hiding this comment

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

ok

@orbeckst
Copy link
Member

@IAlibay I believe your specific comment #161 (review) was addressed but would you mind having a quick look through? Thanks.

@orbeckst
Copy link
Member

@IAlibay if you want to have a look please do over the next few days, otherwise I'll dismiss.

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.

I've made one direct change on the dependencies but otherwise lgtm!

openmmforcefields
cudatoolkit>=11.7.0
cuda-python>=11.7.1
mdanalysis>=2.3.0
Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to have MDAnalysis here, in the end this will go in the docs as "this is how you install this".

I have moved pytest things into test_dependencies though, because as far as I'm aware they aren't necessary for the standard operations of the code.

@IAlibay IAlibay merged commit 6924548 into MDAnalysis:main Sep 21, 2024
5 checks passed
@talagayev
Copy link
Member Author

Nice, thank you @orbeckst and @IAlibay for accepting and helping with this MDAKits entry :)

possum

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mdakit About an MDAKit. new create a new MDAKit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants