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

Upgrade to Conda Build 3 (finally) and Move to Azure DevOps #177

Merged
merged 69 commits into from
Jan 9, 2020

Conversation

Lnaden
Copy link
Contributor

@Lnaden Lnaden commented Nov 26, 2019

After a bunch of testing, this is the working (at least in dry runs) upgrade to the Omnia conda-build infrastructure. This replaces #154 and does the following things:

  • Total overhaul of the Conda-Build-All script to use Conda Build 3+ API
  • Drops Travis and AppVeyor and replaces with Azure DevOps
  • Can be integrated into conda-recipes repo with the goal of rendering this obsolete (topic for discussion)
  • Nightly Builds established through same YAML as non-nightly builds
  • Uses our new docker images from the omniamd Docker Group for linux
  • Splits the Python and CUDA builds 1 per combination of Python {2.7, 3.7, 3.8} and CUDA {8.0, 9.0, 9.1, 9.2, 10.0, 10.1, 10.2}. This results in a large number of builds, but each one is specialized at least and we don't have to fight timeout.
  • The ability to ignore and include explicit Jinja keys in the recipe

You can see the output CI stuff here: https://dev.azure.com/lnaden/conda-dev-recipes/_build/results?buildId=72

Outstanding Issues

  • I'm still using the --dry-run flag and have not adjusted the OpenMM build.sh and build.bat scripts.
  • I am currently only building OpenMM in the CUDA builds through the jinja include/exclude logic I added. However, how do we want to handle OpenMM builds without CUDA?
  • What do we want to call the OpenMM builds which are not built with CUDA
  • I need to actually test a proper build and upload with the script, although the majority of that logic should not have changed.
  • we will need to transfer the BINSTAR_TOKEN to Azure from Travis and/or AppVeyor

Theoretical Issue closures

This PR converts the `conda-build-all` script to be compatible with
Conda Build 3 and Conda 4.4, using the API as much as I can. Its
imperfect due to the imperfect API, but I think I managed to get what
we need without high-maintenance work arounds.

In theory, this should make the conda-build all script much more stable
and we can switch back to using the `conda-forge` docker image as a
base after some iterations

We've kicked this can down the road about as far as it can go at this
point so I think its time to implement a proper fix.

All existing functionality should remain unchanged from how the script
is used and invoked.

Complete list of everything I tweaked:
* Simplify imports to minimal set
* Import only from the Conda(-build) API when possible
* Added docstrings everywhere
* Use metapackage functions directly instead of reading from various metapackage dicts
* Reduce calls to the CLI directly where possible, replacing with API
* Convert previous multi-python multi-numpy build-mixing to Conda Build 3 Variant system
    * Variants compiled from command line, still supports `--python` and `--numpy`
* Build enumeration automatically checks for duplications
* Still uses old package string styles instead of hashed dependencies for checking if package already on Anaconda (Will need to convert at some point)
* Adds ability to check if package is already built locally, and can be overridden with `--rebuild`
* Preserved checking against specific channels, both at command line and meta.yaml level
* Package building handled by API instead of CLI
* Functions more generalized to allow importing the conda-build-all script if need be
* Switched all print statements to the Python 3 `.format()` mini-language over `%` formatting
* Minimized amount of excess data passed between functions, handled mostly by Metadata now
* Added ability for built and uploaded tarballs to be removed to save build space (e.g. Travis-CI) through the `-c` or `--clean` command line arg. This is False by default (related: omnia-md/conda-recipes#877)
* Unpinned the conda and conda-build versions from the scripts. Will need to make changes to the Docker Image though

Related issues: omnia-md/conda-recipes#799
Potentially resolved issues from these changes:
* omnia-md/conda-recipes#712
* omnia-md/conda-recipes#403 (not likely, but something to consider)
Fix clashes
Fix Travis
Time to try!
After a bunch of shenanigans later, I think its finally ready
@Lnaden
Copy link
Contributor Author

Lnaden commented Nov 26, 2019

And cc @jchodera @j-wags @peastman because I'll need your input to finish. At this point, it should be working from a technical stand point, we just need to finalize all the build flags and conditions.

@Lnaden Lnaden closed this Nov 26, 2019
@Lnaden
Copy link
Contributor Author

Lnaden commented Nov 26, 2019

why.. why did that close it? It should stay open

@Lnaden Lnaden reopened this Nov 26, 2019
@Lnaden
Copy link
Contributor Author

Lnaden commented Nov 26, 2019

@jaimergp Continuing our conversation (this is just copy from the slack):

Whatever the default main label is listing, but it shouldn’t matter anyway.

There is one problem I see with this is that we will still need to build a osx version since the current setup I have is OpenMM building only on CUDA images/installs.

@Lnaden
Copy link
Contributor Author

Lnaden commented Nov 26, 2019

To that end, we will need to figure out a build string for OpenMM which works with or without the CUDA_SHORT_VERSION jinja being set.

@jaimergp
Copy link
Collaborator

Can we use nocuda when CUDA_SHORT_VERSION is not set?

@Lnaden
Copy link
Contributor Author

Lnaden commented Nov 26, 2019

Sure, I just need to figure out the Jinja, and we still need to make sure that when someone does conda install openmm they get the correct version.

@Lnaden
Copy link
Contributor Author

Lnaden commented Nov 26, 2019

here is the syntax I assembled:

{% if CUDA_SHORT_VERSION is defined %}
{% set CUDA_STR = "cuda" + CUDA_SHORT_VERSION %}
{% else %}
{% set CUDA_STR = "nocuda" %}
{% endif %}

Can then use: string: py{{ py }}_{{ CUDA_STR }}_{{ PKG_BUILDNUM }} later

@jaimergp
Copy link
Collaborator

Do we want to add the package hash ({{ PKG_HASH }}) too? See numpy at conda-forge for an example.

@Lnaden
Copy link
Contributor Author

Lnaden commented Nov 26, 2019

Do we want to add the package hash ({{ PKG_HASH }}) too?

My first thought is "no" for the reason that I explicitly did not want to re-build existing packages which don't need an update. The "Does the package need to be built" checks the output tarball name against the one on the Conda CDN's and skips unless forced. This worked until Conda-build started hashing the dependency pinning in Conda-Build 3.something. To keep that behavior, I intentionally added the filename_hashing=False kwarg in the conda_build.api.render call which is equivalent to the CLI conda build --old-build-string.

However, since we are now dropping 2.7, we need to build for 3.8, and I'm pretty sure most of the 3.7 builds are missing anyways, it may be worth dropping that and letting packages build with the hashes now once again. We would still have to manually add it to the string for OpenMM since we are manipulating it anyways, but we could!

tl;dr: maybe the answer should be "yes?" Thoughts?

@jchodera
Copy link
Member

jchodera commented Dec 3, 2019

Apologies for not being able to review this earlier! Reviewing this now, since it looks we need to put this into production.

@jchodera
Copy link
Member

jchodera commented Dec 3, 2019

Actually, @jaimergp and I will meet about this tomorrow morning and should be able to review this then!

Copy link
Member

@jchodera jchodera left a comment

Choose a reason for hiding this comment

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

@Lnaden : Thanks so much for doing this! @jaimergp have gone through this.

Here is a summary of our discussion for consideration in future updates:

  • Supported Python versions: conda-forge currently officially supports Python 2.7, 3.6 and 3.7, so we should try to support these if possible. They do have a bot adding 3.8 support, so we should add that as well. I'm certainly in favor of dropping Python 2.7 ASAP, but it would be best if we can keep in sync with what conda-forge supports.
  • Azure Devops vs GitHub Actions: Once the MolSSI CMS Cookiecutter is updated to use GitHub Actions, it would be useful to migrate from Azure Devops to GitHub Actions so that it is easier for folks to just deeply understand one continuous integration system.
  • TeXLive: We stopped building the docs a little while ago, but it's great if we can continue to have TeXLive in our images if we can resurrect the docs building in the recipe (or a separate package). If we can't get it to work, we can likely drop TeXLive from our images.
  • BINSTAR_TOKEN: Since this is active under your Azure account, you'll need to pull the current token from https://anaconda.org/omnia-dev/settings/profile (where you are an owner of omnia-dev no). When we change this to GitHub Actions later, we can see if there is some sort of institutional account we can use.
  • Build strings: I like having a nocuda label for variants that do not have CUDA support built in, though there should always be a default package with the latest CUDA (if available) on main. There's also no point in nocuda for osx if we're not building CUDA variants.
  • Windows builds: I am super excited to have CUDA builds for Windows!
  • Updating with conda-forge: There are a mess of ~/.conda_configs/ files that appear to have been generated with the conda-forge tooling, but how do we update these going forward to stay in sync with the conda-forge infrastructure? Any thoughts on how we make this sustainable?
  • conda-build-all: Is this script something we should make external, or try to harmonize with the build_all.py from conda-forge, or the CI build infrastructure in its entirety?

@jaimergp will work with you on updating the openmm recipe to bring it into alignment with the conda-forge recipe in the areas where this is needed.

Overall, this is great, and we can merge and move forward with iterative improvements!

@peastman
Copy link
Contributor

peastman commented Dec 3, 2019

conda-forge currently officially supports Python 2.7, 3.6 and 3.7, so we should try to support these if possible.

The official end of support date for Python 2.7 is in less than a month, so I don't think it's worth putting in any effort to try to support it. Also, the next OpenMM release will not support Python 2.

@jaimergp
Copy link
Collaborator

jaimergp commented Dec 3, 2019

I'll leave this resource here for future reference. IBM provides packages for cudatoolkit and cudatoolkit-dev for both linux64 and ppc64le. We can't use these in conda-forge because this is an external channel, but might be useful for omnia builds at some point?

https://public.dhe.ibm.com/ibmdl/export/pub/software/server/ibm-ai/conda/#/

More info: conda-forge/openmm-feedstock#8 (comment)

@jaimergp
Copy link
Collaborator

jaimergp commented Dec 3, 2019

One more thing, maybe for a separate PR: PowerPC builds. Nvidia provides base images here. Do we want to include these builds here too?

@Lnaden
Copy link
Contributor Author

Lnaden commented Dec 4, 2019

currently officially supports Python 2.7, 3.6 and 3.7,

Yes, but Python 2.7 dies on Jan 1 2020, by the time we get all this hammered out, we may as well just not support it. They are also working on phasing out 3.6 and once 3.8 is stable on their builds, 3.6 will drop. I would not say they are supporting 3.6 as much as they are phasing it out. They could drop it any day as far as I know.

Azure Devops vs GitHub Actions

That is something we can hash after getting the build system upgraded first, one thing at a time.

TeXLive

I would love to drop this, it adds a fair amount of complexity to the image since I have to build a parallel glibc for the newer (2018+) TeXLives to support.

BINSTAR_TOKEN: Since this is active under your Azure account,

Its under my azure account so I could test without spamming everyone while I tried a bunch of things and in case it did not work. My plan was to merge this in, and then create a group Azure account which all Omnia devs have full control over. So hopefully we can resolve this without having to move the binstar token between owners.

Build strings: I like having a nocuda label for variants that do not have CUDA support built in, though there should always be a default package with the latest CUDA (if available) on main. There's also no point in nocuda for osx if we're not building CUDA variants.

This is something I'm not sure how to best handle. I don't know how to make it so when someone does conda install openmm it just "gets" the right package with CUDA support. Also, the more complexity we add in the logic for the build string, the harder it is to program. Right now I have Jinja header for setting the CUDA string, I could add another, but it would be good to hammer out exactly what we want the string to be and under what conditions. I do think we should start adding in the PKG_HASH to packages going forward for all omnia packages. (see #177 (comment))

Updating with conda-forge

There are a bunch of them, but I wrote most of them. They are just simple YAML dicts with 1 or 2 options. I have a generator script I have added in a recent change locally I will be pushing.

conda-build-all...

I would be very, very careful on how much extra work to put into this. I tried to make this so that it pays the technical debt we have accrued over the years while requiring minimal maintenance in the future (hence why I try to use the Conda Build API everywhere I can). Staged recipes is a very special repo from Conda-Forge and I am very weary of trying to mimic conda-forge's build system because it makes alot of assumptions we don't (channel rigidity, build images, etc) and has a bunch of active devs which we cannot commit in equal parts.

Overall, I am going to make some changes on my end, and work with @jaimergp to get OpenMM building on my local builds (to at least ensure it works), then we can move forward with merging, tweaking, and fixing. Sound good?

Add a generator script to the YAML configs
Re-add Python 3.6
Block PyCharm .idea's folder from being added to git
@jchodera
Copy link
Member

jchodera commented Jan 6, 2020

@Lnaden @jaimergp: Happy New Year, and welcome back from the holidays!

We desperately need to complete the refurbishment of our omnia build infrastructure, get nightly builds working again, and release the conda-forge version of OpenMM. What else needs to be done here, and is there any way I can help?

@jchodera
Copy link
Member

jchodera commented Jan 6, 2020

(Notably, we haven't pushed an omnia-dev nightly build of openmm for two months now, and we've reached a point where we're going to need the new nightlies once openmm/openmm#2511 is merged until we can get that into a release.)

@jaimergp
Copy link
Collaborator

jaimergp commented Jan 6, 2020

I am happy to take over whenever @Lnaden gives the OK!

@Lnaden
Copy link
Contributor Author

Lnaden commented Jan 6, 2020

I think it might be best to hand it off. I'm not sure when I can finish this in the short term. @jaimergp do you want to have another chat later today or tomorrow afternoon and we can go over all the changes/mechanisms this will introduce as well as what needs done to finish it?

@Lnaden Lnaden changed the base branch from master to conda-build3-v2 January 7, 2020 17:49
@Lnaden
Copy link
Contributor Author

Lnaden commented Jan 7, 2020

@jaimergp, I have made some last changes to this, I think its ready to be merged in. It will merge to conda-build3-v2 for a branch instead of master, so you should be able to take control of it from there. I am getting the Azure DevOps org set up now

@jaimergp
Copy link
Collaborator

jaimergp commented Jan 8, 2020

@Lnaden LGTM!

@jaimergp
Copy link
Collaborator

jaimergp commented Jan 8, 2020

I might need admin access to this org too.

@Lnaden
Copy link
Contributor Author

Lnaden commented Jan 8, 2020

@jaimergp I have added you to the maintainers for this repo, you should be able to to merging and whatnot after accepting!

@jaimergp
Copy link
Collaborator

jaimergp commented Jan 9, 2020

Do we need approval from Peter too or can I just merge this to the dev branch?

@Lnaden
Copy link
Contributor Author

Lnaden commented Jan 9, 2020

Na, you can just merge this since its going to a branch called conda-build3-v2 and not master

@jaimergp jaimergp merged commit 8ca6c93 into omnia-md:conda-build3-v2 Jan 9, 2020
@Lnaden
Copy link
Contributor Author

Lnaden commented Jan 9, 2020

Awesome

@jaimergp jaimergp mentioned this pull request Jan 9, 2020
7 tasks
@jchodera
Copy link
Member

jchodera commented Jan 9, 2020

Do we need approval from Peter too or can I just merge this to the dev branch?

FYI, Peter hasn't been much involved in the conda build infrastructure, but has generously offered to help maintain it going forward.

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.

4 participants