conda-forge / ambertools-feedstock

A conda-smithy repository for ambertools.
BSD 3-Clause "New" or "Revised" License
8 stars 14 forks source link

AmberTools23.6 and internal packmol #137

Closed SSchott closed 5 months ago

SSchott commented 6 months ago

Checklist

See #136

conda-forge-webservices[bot] commented 6 months ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

SSchott commented 6 months ago

Hey @mattwthompson , thanks for your quick reply. As I have never modded a feedstock, maybe you can have a look.

mattwthompson commented 6 months ago

Are you trying to make a new build? There are no changes to the source or new patches added

Some helpful documentation can be found at

SSchott commented 6 months ago

Are you trying to make a new build? There are no changes to the source or new patches added

Some helpful documentation can be found at

* https://conda-forge.org/docs/maintainer/updating_pkgs/#updating-recipes

* https://conda-forge.org/docs/maintainer/adding_pkgs/#the-recipe-metayaml

* https://docs.conda.io/projects/conda-build/en/stable/resources/define-metadata.html

Again, first time modifying a conda recipe, so while I read those docs, some gut feeling is involved. Isn't it pulling the patches from amber by calling the update script? If I understood it correctly, that means that it would need a new build number, but I might be wrong (?).

mattwthompson commented 6 months ago

Oh, yeah, sorry, I forgot AmberTools does off-standard things with source and patches

dacase commented 6 months ago

What is being proposed is fine by me.

jaimergp commented 6 months ago

If packmol needs to be vendored, we need to guarantee that:

jaimergp commented 6 months ago

@conda-forge-admin, please rerender

jaimergp commented 6 months ago

We might need to come up with a different workaround at

https://github.com/conda-forge/ambertools-feedstock/blob/f14f8759cb85823b5bd1a62403e7167be1f4469f/recipe/build.sh#L49

Or maybe this X11 juggling is not needed anymore.

SSchott commented 6 months ago

Hi @jaimergp Thanks for the comments!

jaimergp commented 6 months ago

Packmol uses MIT, so we are fine there

Yes, but MIT (and many others) require license redistribution along with the package itself, so that's why we must explicitly list it if vendored.

Another package that I noticed you are pulling from git is parmed.

In the past, @swails has helped us tag release ParmEd commits so they are released in the regular way, so this should be accurate.

Given that the software is used in a scientific setting, if anyone uses parmed of AmberTools version XXX as a citation, if such a citation would be accurate then?

I don't know what "accurate" means here, but if we are talking about experiment reproducibility, a citation won't be enough to guarantee it. I hope those researchers are providing lockfiles or other type of metadata to actually share their runtime environment.


In general, my concern with this monolithic ambertools distribution model is that it assumes folks won't mix it with other things in the same environment. However, some workflows require the use of ambertools as a library instead of an application. Soft-vendoring (Python) packages like ambertools does (with no namespacing), risks clobbering with other packages in the same environment. In this way, it makes coexistence with other packages in the same environment problematic. I'd prefer a setup where ambertools releases are assembled from individual blocks that are released separately, but I understand that's a burden for maintainers. If that's not possible, then the docs should recommend installing ambertools in its own environment.

jaimergp commented 6 months ago

We might need to come up with a different workaround at

https://github.com/conda-forge/ambertools-feedstock/blob/f14f8759cb85823b5bd1a62403e7167be1f4469f/recipe/build.sh#L49

Or maybe this X11 juggling is not needed anymore.

Ah, this other PR has the changes needed to fix the osx-arm64 failures I think:

https://github.com/conda-forge/ambertools-feedstock/pull/133/commits/b65b4036f96d29eb3188ed7057c75b89900b6ef4

Basically, prepend CONDA_SUBDIR=$target_platform to that line.

dacase commented 6 months ago

On Wed, Mar 20, 2024, jaimergp wrote:

In general, my concern with this monolithic ambertools distribution model is that it assumes folks won't mix it with other things in the same environment. However, some workflows require the use of ambertools as a library instead of an application. Soft-vendoring (Python) packages like ambertools does (with no namespacing), risks clobbering with other packages in the same environment. In this way, it makes coexistence with other packages in the same environment problematic. I'd prefer a setup where ambertools releases are assembled from individual blocks that are released separately, but I understand that's a burden for maintainers. If that's not possible, then the docs should recommend installing ambertools in its own environment.

Thanks for your comments. First, we do explicitly recommend that people downloading the AmberTools conda package do so in a separate envionment:

https://ambermd.org/GetAmber.php#ambertools

However, not everyone will do that, and separate environments can be a pain.

It would be great if contributors to this thread could indicate which parts of AmberTools that would be the best candidates for an initial division into separate packages. Or, to much the same effect: what conflicts are you seeing if the "monolithic" AmberTools is loaded into the one of your current environments?

I don't see a path to complete breakdown of AmberTools into pieces, but we might solve most of the problems by splitting off things like antechamber, leap, packmol-memgen, parmed, cpptraj, pytraj, parameter files, (etc?) into separate packages. One challenge will be to make the non-conda (source code) installation process look as much as possible like the conda installation.

I'm willing to help out here. I don't see this as happening in time for AmberTools24 (out next month), but I agree that Amber developers should be putting more effort in this direction.

....dac

mattwthompson commented 6 months ago

@dacase great question - I'll speak in part on behalf of OpenFF in a separate thread #138

SSchott commented 6 months ago

Thanks for your comments. First, we do explicitly recommend that people downloading the AmberTools conda package do so in a separate envionment: https://ambermd.org/GetAmber.php#ambertools However, not everyone will do that, and separate environments can be a pain. It would be great if contributors to this thread could indicate which parts of AmberTools that would be the best candidates for an initial division into separate packages. Or, to much the same effect: what conflicts are you seeing if the "monolithic" AmberTools is loaded into the one of your current environments? I don't see a path to complete breakdown of AmberTools into pieces, but we might solve most of the problems by splitting off things like antechamber, leap, packmol-memgen, parmed, cpptraj, pytraj, parameter files, (etc?) into separate packages. One challenge will be to make the non-conda (source code) installation process look as much as possible like the conda installation. I'm willing to help out here. I don't see this as happening in time for AmberTools24 (out next month), but I agree that Amber developers should be putting more effort in this direction. ....dac

I agree that in principle we could go in that direction, but that would have to be intentional and disclosed to the maintainers and users clearly. Something like a "rolling release", which would be the conda implementation, and the "long term support" if you want. But then we might want to have a closer look at naming schemes for example.

Seems like I opened a pandora's box here, but I also think is a very interesting discussion.

@jaimergp I added the CONDA_SUBDIR var suggestion here as well

jaimergp commented 6 months ago

Thanks! I think we are almost there. Since we confirmed that external packmol is a subset of functionality of the vendored one, we can safely add packmol to run_constrained (same thing as ambermini there) to prevent files replacement/ clobbering.

Hopefully this will pop up in Google / search engines if someone runs into a conda solver conflict with ambertools and packmol (dropping all the terms together here for SEO friendliness :P).

SSchott commented 6 months ago

I think this should be ready to go

SSchott commented 5 months ago

Given that @dacase announced Amber24 today, could this be merged, so we can move the page to the upgraded version?

mattwthompson commented 5 months ago

@conda-forge-admin, please rerender

conda-forge-webservices[bot] commented 5 months ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

mattwthompson commented 5 months ago

This doesn't block an AmberTools 24 release - builds don't need to be sequential

github-actions[bot] commented 5 months ago

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

Thus the PR was passing and merged! Have a great day!