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

Follow up of #21: convert to noarch #26

Merged
merged 13 commits into from
Jul 7, 2022
Merged

Conversation

HeavyTony2
Copy link
Contributor

@HeavyTony2 HeavyTony2 commented May 17, 2022

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@conda-forge-linter
Copy link
Contributor

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

I was trying to look for recipes to lint for you, but it appears we have a merge conflict.
Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

@conda-forge-linter
Copy link
Contributor

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

I wanted to let you know that I linted all conda-recipes in your PR (recipe) and found some lint.

Here's what I've got...

For recipe:

  • There are too few lines. There should be one empty line at the end of the file.
  • noarch packages can't have selectors. If the selectors are necessary, please remove noarch: python.

@HeavyTony2
Copy link
Contributor Author

@conda-forge-admin, please rerender

@github-actions
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/anndata-feedstock/actions/runs/2340651858.

@HeavyTony2
Copy link
Contributor Author

@flying-sheep
To answer to that question:
#21 (comment)

Normally, we're not suppose to put selectors to package with noarch. So, it triggers a lintin-error as you can notice.
What are the options then?

  1. Keeping the dependancies for all python versions to keep the noarch. typing_extensions has no other dependancies than python and pip. importlib_metadata requires typing_extension and zipp, which doesn't require another dependencies than python and pip. In other words, keeping them in the recipe wouldn't make a notable difference for users with Python 3.8 and plus. ALSO: Noarch tarball would be 76kb. 76kb+3.8kb for importlib_metadata tarball + 27kb for typing_extensions is stil less than the current 150kb.
  2. Keeping it separated instead of noarch to manage that specificity. However, this is already a noarch package on Pypi. It's then more convenient to install it from Pypi than Conda-Forge.
  3. Keeping the selectors for the two packages and keeping the noarch. However, it would trigger a linting error for earch update until Python 3.7 stops being supported by Conda-Forge.

@flying-sheep
Copy link
Contributor

What is the linting rule about? Will the selectors be ignored? If so, they should be removed. If they are functional, I don’t understand what the linting rule is about, as keeping the selector would be semantically ideal.

@ivirshup
Copy link
Contributor

I think we'll drop python 3.7 with the next release cycle.

Does that simplify things a bit here?

@HeavyTony2
Copy link
Contributor Author

@ivirshup So, in that case, it would be

requirements:
  host:
    - python >=3.8
    - pip
    - flit-core
    - setuptools_scm
    - pytoml
  run:
    - python >=3.8 
    - pandas >=0.23.0
    - scipy >=1.4
    - h5py
    - numpy >=1.16.5
    - natsort
    - packaging >=20

@HeavyTony2
Copy link
Contributor Author

@flying-sheep I found out the problem: If I build the package on python 3.7, our two deps will be listed in the run list. However, they won't if the package is built on a higher version. The conda-forge noarch packages are currently built on Python 3.10 . It means Python 3.7 users won't have the deps installed because they won't be listed in the recipe.
So the choice is between having the package only avalaible for 3.8+ or having this two deps for all.

@HeavyTony2
Copy link
Contributor Author

ping @conda-forge/anndata

@ivirshup
Copy link
Contributor

ivirshup commented Jun 7, 2022

@HeavyTony2 I think you'd need to bump that h5py dependency to - h5py >=3.0 as well.

But I wouldn't want to drop 3.7 for the conda build until anndata drops support for 3.7.

@conda-forge-linter
Copy link
Contributor

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.

@HeavyTony2
Copy link
Contributor Author

HeavyTony2 commented Jun 8, 2022

@HeavyTony2 I think you'd need to bump that h5py dependency to - h5py >=3.0 as well.

But I wouldn't want to drop 3.7 for the conda build until anndata drops support for 3.7.

@ivirshup Updated for h5py. I even left a note about the two 3.7 specific dependancies.

@HeavyTony2
Copy link
Contributor Author

@conda-forge-admin, please rerender

@HeavyTony2
Copy link
Contributor Author

ping @conda-forge/anndata

@HeavyTony2
Copy link
Contributor Author

@conda-forge-admin, please rerender

@HeavyTony2
Copy link
Contributor Author

ping @conda-forge/anndata

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