conda-forge / ambertools-feedstock

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

Procedure for creating updates to AmberTools23? #117

Closed dacase closed 1 year ago

dacase commented 1 year ago

Comment:

Hi: Pull requests #113 and #116 will require changes to the AmberTools23 source. There seem to be two possible ways to update the conda packages:

  1. have ambertools-feedstock access the update.xxx files at ambermd.org, starting from the original (23.0) AmberTools tar file.
  2. Create an updated, complete tar file, and have the conda build script pull that.
  3. something else??

Option 2 seems easier, but I don't remember doing that in the past. I'm happy to create the updated tar file.

mattwthompson commented 1 year ago

Building from an official release asset (i.e. a single tarball) is preferred - if there are updates to a package, they're usually reflected in a new release with a different version. conda-forge might have a policy that states this preference more strongly, I can't find the official stance on it.

Applying patches to an existing release is possible but not elegant. You can see some examples of this in recipe/patches/

jaimergp commented 1 year ago

We have been ok with getting the updates as patches for a while (see beginning of build.sh). Full archives are preferred, though.

hainm commented 1 year ago

I think full archives are just easier, just a matter of providing a new link.

mikemhenry commented 1 year ago

Patches work, but some of the improvements I think will be useful to the whole AmberTools community so creating a 23.1 version I think would be best.

dacase commented 1 year ago

OK: I have new, updated tar file for trying to get things to work with python 3.11:

https://ambermd.org/downloads/AmberTools23.3.tar.bz2

(This takes the place of AmberTools23_rc6 in the current build.) Can someone try to build a conda package with python 3.11 with this?

I've tested the code with Amber's CI procedure (jenkins), without any problems. But we are not currently testing python 3.11, so it's possible there will be a glitch.

A note about numbering: updates 1 and 2 did not require new conda packages: they involved things like cuda changes, and we have not been building for cuda in conda-forge. So this change (assuming it works) would become version 23.3.

I promise to study the final pull request at ambertools-feedstock to see exactly what needs to be done. I see several tasks: change the location of the source tar file; update the version number to 23.3; ask to build for python 3.11 as well as some selection of earlier pythons.

....thanks...dave

mattwthompson commented 1 year ago

@dacase I've started this in #118 with some verbose comments that are hopefully useful for understanding the changes. No guarantees it works on the first try 🤞

swails commented 1 year ago

The challenge with using full archives is that they can only be created and uploaded by Dave (compare that to update files which can be raised as a PR by any developer, then merged/synced by Dave).

I know I'm late to this conversation (I was on vacation last week), but I don't think it's a good idea to split how we update source code in conda-forge vs. how everyone else gets it. I don't think it's any easier to do it that way given that build.sh already encodes the logic to update to a specific version. You could argue that you get increased safety by checking the hash of the updated package, but the counterargument is that you're no longer guaranteeing that the source tarball matches what non-conda users will have (not to mention the 1.5 GB tarballs that will need to be stored for each version on the server, which will start to be impactful if/when hosting is moved to cloud infrastructure).

dacase commented 1 year ago

Here's a minor, practical, problem with Jason's suggestions; I'm sure there's a good way around this, but I'm not sure exactly how to proceed.

To continue to follow the "update" route requires (at least in the current form of build.sh) that the updates be posted to the Amber web site, where they will be picked up by users building from source. Once an update is posted, it is "out there" and cannot be withdrawn.

The "problem" is that the current proposed update file works fine in CI that we do internally in Amber, but the conda-forge scripts fail with the same cpp files. So, we need to fix the conda-forge build before actually posting an update.

I see two possible (easy) solutions:

  1. As now, create a full tarball for conda-forge testing. This would just be considered as a temporary file, and would be discarded once a update file that works everywhere is available.
  2. Establish a "hidden" location for update files for use during conda-forge testing. This allows us to still use the update_amber-like functionality during testing. Once things work everywhere, the update files can be posted to the official Amber web page and be used by those building from source code.

The second option is probably safer, and just requires some thought about the "hidden" site should be established.

swails commented 1 year ago

I think it's worth trying to update the conda-forge build script to remove pre-cythonized files and just rebuild pytraj entirely.

hainm commented 1 year ago

I think it's worth trying to update the conda-forge build script to remove pre-cythonized files and just rebuild pytraj entirely.

I agree with @swails about this. (I've thought about that too).

dacase commented 1 year ago

A possible problem with the proposals by Jason and Hai: this would really separate the source code build procedure (which does not require cython) with the conda-forge build procedure (which would depend on a particular version of cython.) I'd rather keep the two builds in sync with each other -- i.e. that neither would depend on cython.

hainm commented 1 year ago

@dacase yeah, you made a good point too.

dacase commented 1 year ago

As a test, I've recreated https://ambermd.org/downloads/AmberTools23.3.tar.bz2 -- this now has a proposed fix to the problem seen in #118. It would probably be worth re-trying #118 -- this has a genuine fix to core/c_dict.cpp, but of course that might not be the only problem.

dacase commented 1 year ago

My summary: it's really an advantage to Amber to use update_amber to apply updates to create "dot" releases. Unless conda-forge really requires a new (600 MB) tar file to be created for each minor release, we should continue as we have in the past. Big win is that this helps ensure full compatibility with versions created by users building from source.

I'm going to close this issue, but it can always be re-opened if needed.