TileDB-Inc / tiledbsoma-feedstock

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

Only upload binaries for builds on "main" branch #11

Closed jdblischak closed 1 year ago

jdblischak commented 1 year ago

Motivation: Maintainers want to be able to easily run CI on experimental changes in branches (running the CI on your fork requires setting up your own Azure infrastructure). We may also add a branch to run nightly builds

Documentation: https://conda-forge.org/docs/maintainer/conda_forge_yml.html#upload-on-branch

CC: @johnkerl @Shelnutt2

Context: https://github.com/single-cell-data/TileDB-SOMA/issues/1242 Note that this is a pure feedstock infrastructure update, so there's no need to modify the recipe (nevermind...)

Context: https://github.com/single-cell-data/TileDB-SOMA/issues/1242

jdblischak commented 1 year ago

I'm confused why the linux build failed. Its dependency anndata tried to import setuptools_scm, which isn't available in the test environment (since its a build time requirement)

+ python -c 'import tiledbsoma; print(tiledbsoma.pytiledbsoma.version())'
Traceback (most recent call last):
 File "/home/conda/feedstock_root/build_artifacts/tiledbsoma_1681242029715/_test_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_place/lib/python3.7/site-packages/anndata/_metadata.py", line 25, in <module>
 from setuptools_scm import get_version
ModuleNotFoundError: No module named 'setuptools_scm'

However, the anndata recipe itself only installs setuptools_scm in the build environment, and it just recently passed just fine (I confirmed its test env also didn't have setuptools_scm installed but had no trouble importing anndata). Maybe the problem is we are trying to access the version of anndata?

https://github.com/conda-forge/anndata-feedstock/blob/26edaa7b651d1d04bca7db2592710923f30fb170/recipe/meta.yaml#L22 https://github.com/scverse/anndata/blob/0.9.0/anndata/_metadata.py#L25 https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=687857&view=logs&j=656edd35-690f-5c53-9ba3-09c10d0bea97&t=e5c8ab1d-8ff9-5cae-b332-e15ae582ed2d&l=246

jdblischak commented 1 year ago

I couldn't reproduce the error locally:

mamba create --yes -n test-anndata anndata
mamba activate test-anndata
python -c 'import anndata; print(anndata.__version__)'
## 0.9.0
mamba list setuptools
## # Name                    Version                   Build  Channel
## setuptools                67.6.1             pyhd8ed1ab_0    conda-forge
jdblischak commented 1 year ago

I also couldn't reproduce the error in the linux build of the anndata-feedstock. I explicitly added print(anndata.__version__) to the tests, and it worked fine. This test env does not have setuptools_scm installed, though it does have both importlib-metadata and importlib_metadata

import: 'anndata'
import: 'anndata'
+ pip check
No broken requirements found.
+ python -c 'import anndata; print(anndata.__version__)'
0.9.0
johnkerl commented 1 year ago

Looking good @jdblischak !! :)

jdblischak commented 1 year ago

I also compared the code changes introduced between anndata 0.8.0 (which works in our CI) and 0.9.0 (which unexpectedly fails)

https://github.com/scverse/anndata/compare/0.8.0...0.9.0

The offending file, _metadata.py, only had one minor change:

https://github.com/scverse/anndata/compare/0.8.0...0.9.0#diff-d1457d5b68c2430a421385d8334f8113fa6d75c95df7f2930226eef216543183

Which is related to one of the exceptions. So the question is, why can't the following be run in the test environment for our recipe? Both importlib-metadata and importlib_metadata are installed in the test environment

from importlib.metadata import metadata
jdblischak commented 1 year ago

I figured it out. It's a Python 3.7 compatibility problem. anndata 0.9.0 dropped support for Python 3.7

https://github.com/scverse/anndata/pull/820

And I could reproduce the error locally:

mamba activate test-anndata
python --version
## Python 3.11.3
python -c 'import anndata; print(anndata.__version__)'
## 0.9.0
mamba install python=3.7
python --version
## Python 3.7.12
python -c 'import anndata; print(anndata.__version__)'
## Traceback (most recent call last):
##   File "~/mambaforge/envs/test-anndata/lib/python3.7/site-packages/anndata/_metadata.py", line 25, in <module>
##     from setuptools_scm import get_version
## ModuleNotFoundError: No module named 'setuptools_scm'
## 
## During handling of the above exception, another exception occurred:
## 
## Traceback (most recent call last):
##   File "<string>", line 1, in <module>
##   File "~/mambaforge/envs/test-anndata/lib/python3.7/site-packages/anndata/__init__.py", line 3, in <module>
##     from ._metadata import __version__, within_flit
##   File "~/mambaforge/envs/test-anndata/lib/python3.7/site-packages/anndata/_metadata.py", line 30, in <module>
##     from importlib.metadata import metadata
## ModuleNotFoundError: No module named 'importlib.metadata'
johnkerl commented 1 year ago

cc @Shelnutt2 -- @jdblischak I don't know a priori whether it's better to pin anndata < 0.9 in Conda or in the TileDB-SOMA repo's setup.py ... as your surely know, we do have customers on Python 3.7 (e.g. via old RHEL) to maintain support for

Shelnutt2 commented 1 year ago

cc @Shelnutt2 -- @jdblischak I don't know a priori whether it's better to pin anndata < 0.9 in Conda or in the TileDB-SOMA repo's setup.py ... as your surely know, we do have customers on Python 3.7 (e.g. via old RHEL) to maintain support for

Likely needed in both. Pins in setup.py are not "really" for conda, the conda feedstock needs to list the dependencies and pins.

jdblischak commented 1 year ago

I'm also working on fixing the problem upstream in https://github.com/conda-forge/anndata-feedstock/pull/28. But yes, probably also a good idea to pin it in setup.py for anyone relying on pip or source installs

have customers on Python 3.7 (e.g. via old RHEL) to maintain support for

Hope those customers don't need the latest updates from the scverse packages. Both anndata (https://github.com/scverse/anndata/pull/820) and scanpy (https://github.com/scverse/scanpy/pull/2447) have dropped support for Python 3.7

flying-sheep commented 1 year ago

Those customers shouldn’t rely on their system provided Python since they are using a super LTS distro. The whole ecosystem moves faster than that.

jdblischak commented 1 year ago

Not to mention that Python 3.7 will stop receiving security patches in a few months (2023-06-27 to be exact)

https://devguide.python.org/versions/ https://www.python.org/downloads/

jdblischak commented 1 year ago

I cleaned up my commit history. Once the CI passes, I plan to merge the PR

Note that the py selectors for anndata will no longer necessary once my repodata patch PR is merged (https://github.com/conda-forge/conda-forge-repodata-patches-feedstock/pull/433), but we can clean that up in the future. For now it also serves as documentation that anndata >= 0.9 is incompatible with Python 3.7

johnkerl commented 1 year ago

Likely needed in both. Pins in setup.py are not "really" for conda, the conda feedstock needs to list the dependencies and pins.

https://github.com/single-cell-data/TileDB-SOMA/pull/1252 FYI

flying-sheep commented 1 year ago

Not to mention that Python 3.7 will stop receiving security patches in a few months (2023-06-27 to be exact)

devguide.python.org/versions python.org/downloads

To be fair, that’s not relevant for those distros. They’ll backport all security patches diligently. Reasons to use them is for a long term stable environment where nothing but security patches come in. (E.g. if you used Python for some server or microservice that isn’t expected to grow new features or otherwise gain new dependencies in the coming years). That’s why my argument was “if you need newest anything, environments that come with CentOS are not for you”

jdblischak commented 1 year ago

To be fair, that’s not relevant for those distros. They’ll backport all security patches diligently.

I'm not sure I follow. Are you saying that CentOS maintainers will backport security patches to Python 3.7 even when the core Python developers are no longer doing it? So there would be unofficial CentOS-only Python 3.7.x patch releases? Python 3.7 is EOL this June. Unless the CentOS maintainers become Python developers, CentOS will be using the final release in the 3.7 series until the LTS distro is itself EOL. In other words, after Python 3.7 hits EOL, there will be no more security patches to backport

flying-sheep commented 1 year ago

Are you saying that CentOS maintainers will backport security patches to Python 3.7 even when the core Python developers are no longer doing it? So there would be unofficial CentOS-only Python 3.7.x patch releases?

Exactly. Until the EOL of that CentOS version.

I think you’re right in that there are practical limitations to that approach (security patches made to 3.8 and later might not apply to the 3.7 tree and might therefore have to be manually backported) but that’s what people pay Red Hat for.

jdblischak commented 1 year ago

Exactly. Until the EOL of that CentOS version. ...that’s what people pay Red Hat for.

Glad I asked! I learned something new. Thanks for the explanation @flying-sheep!

jdblischak commented 1 year ago

Update: we added a nightly build in a branch in PR #12. If we wanted to re-enable uploading binaries from a branch, we'd need to first clean up .github/workflows/nightly.yml to:

  1. embed the date in the version string (or else be ok with it being X.X.X from the previous release plus "-nightly", and regularly overwriting itself)
  2. use the label "nightlies" so the uploaded binaries don't affect end users
jdblischak commented 11 months ago

FYI I prevented a potential Python 3.7 build error by updating the python pins in the recipe for scanpy. This prevents the situation we had with anndata, where the latest version was installed in the Python 3.7 env for tiledbsoma-py even though it had dropped support for Python 3.7

https://github.com/conda-forge/scanpy-feedstock/pull/8#issuecomment-1693454318

Thus there is no need to update setup.py for tiledbsoma-py. scanpy 1.9.3 should get installed in the Python 3.7 build, and scanpy 1.9.4+ in the Python 3.8+ builds