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

update scikit to 0.20.0 #100

Closed
wants to merge 24 commits into from
Closed

Conversation

k-dominik
Copy link
Contributor

@k-dominik k-dominik commented Mar 16, 2023

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-webservices
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.

@k-dominik
Copy link
Contributor Author

meh, ok for some reason https://pypi.io/packages/source/s/scikit-image/scikit-image-0.20.0.tar.gz doesn't point to the source dist...

@k-dominik
Copy link
Contributor Author

@conda-forge-admin, please rerender

@jni
Copy link
Contributor

jni commented Mar 17, 2023

Thanks for the update @k-dominik! I don't know why the conda-forge bot went on strike. 🤔

I expect the tests are failing because the dependencies in the recipe need to be updated. Here's the current deps:

https://github.com/scikit-image/scikit-image/blob/907bfb71d2c8e4d58c2dffc6056760c1e00d070b/pyproject.toml#L70-L79

ref: https://github.com/scikit-image/scikit-image/blob/907bfb71d2c8e4d58c2dffc6056760c1e00d070b/pyproject.toml#L70-L79

also removed Python 3.7 special pins - Python 3.7 is not longer supported by
skimage as far as I can tell.
@k-dominik
Copy link
Contributor Author

k-dominik commented Mar 17, 2023

Thank you for the quick feedback @jni,

I updated the dependencies as suggested, but there seems to be something off with pypi - the tests fail with:

Downloading https://pypi.io/packages/source/s/scikit-image/scikit-image-0.20.0.tar.gz
INFO:conda_build.source:Source cache directory is: /home/conda/feedstock_root/build_artifacts/src_cache
INFO:conda_build.source:Downloading source to cache: scikit-image-0.20.0_e1076d7dc2.tar.gz
INFO:conda_build.source:Downloading https://pypi.io/packages/source/s/scikit-image/scikit-image-0.20.0.tar.gz
Error: HTTP 404 NOT FOUND for url <[https://pypi.io/packages/source/s/scikit-image/scikit-image-0.20.0.tar.gz>](https://pypi.io/packages/source/s/scikit-image/scikit-image-0.20.0.tar.gz%3E)
Elapsed: 00:00.056455

okay, after looking at this, I realized that the artifact on pypi has scikit_image (-> scikit_image-0.20.0.tar.gz) with an underscore instead of a hyphen. Is this expected?

@k-dominik
Copy link
Contributor Author

k-dominik commented Mar 17, 2023

not sure the required version of meson-python can currently be satisfied as it's an rc... I assume meson-python 0.12.1 won't do?

Is it possible that all the cross-compilation issues in CI are related to that?
Also I'm not quite sure about the numpy 1.20 builds as I diligently specified numpy >=1.21 in the recipe 🤷

@mkcor
Copy link

mkcor commented Mar 17, 2023

not sure the required version of meson-python can currently be satisfied as it's an rc... I assume meson-python 0.12.1 won't do?

I guess @jarrodmillman would know. This minimum version requirement comes from scikit-image/scikit-image#6757.

@stefanv
Copy link
Contributor

stefanv commented Mar 17, 2023

okay, after looking at this, I realized that the artifact on pypi has scikit_image (-> scikit_image-0.20.0.tar.gz) with an underscore instead of a hyphen. Is this expected?

This is a known change. Meson, our new build system, follows the pypa rules for cleaning up sdist names, which setuptools (ironically) never did.

@jni
Copy link
Contributor

jni commented Mar 22, 2023

@k-dominik

not sure the required version of meson-python can currently be satisfied as it's an rc... I assume meson-python 0.12.1 won't do?

This PR to the meson-python-feedstock suggests that those issues maybe should be fixed, but I see that in the recipe you didn't constrain meson-python, so I don't think that's where they come from. Hopefully @stefanv and @jarrodmillman can have a look at the build soon...

@stefanv
Copy link
Contributor

stefanv commented Mar 23, 2023

I think Meson needs the cross files to cross-compile on arm64? See, e.g., the SciPy patch and the Meson docs.

@stefanv
Copy link
Contributor

stefanv commented Mar 23, 2023

@k-dominik I am going to try to get the correct CPU version of ninja in place; if pushing to your PR in conda-forge is considered bad practice, I do apologize, and feel free to remove those patches.

@stefanv
Copy link
Contributor

stefanv commented Mar 28, 2023

Build machine cpu family: aarch64
Build machine cpu: aarch64
Host machine cpu family: aarch64
Host machine cpu: arm64
Target machine cpu family: aarch64
Target machine cpu: arm64

@stefanv
Copy link
Contributor

stefanv commented Mar 28, 2023

@hmaarrfk what does re-rendering do exactly? Looks like it updates to the most recent version of all the forge CI pipelines?

Rerendering is conda-forge’s way to update the files common to all feedstocks (e.g. README, CI configuration, pinned dependencies).

@hmaarrfk
Copy link
Contributor

@stefanv rerendering does 2 things:

  1. "pins" compile time dependencies.
    • You can build the same package with multiple different compile time dependencies. For example, we build for different versions of python, each version is "pinned" in the associated yaml file.
  2. Generates the scripts required for the CIs (there can be multiple providers) to do the work.

In order to not step on each other's toes, I'm going to make small attempts in
#101

Sorry for the noise here.

@hmaarrfk
Copy link
Contributor

I thought scipy had already trailblazed meson + scientific python? is that not true? I thought we were following the build procedure of an other major package? Can we use their recipe as a model?

@hmaarrfk
Copy link
Contributor

I haven't really kept up with how you all managed to build OSX on CIs with meson python. Maybe that can be extended to work on conda-forge? If you point me to it I can likely help with that.

I noticed this bug report opened by isuruf (conda-forge core member) mesonbuild/meson-python#321

  • To get windows to work you will need: 7390e30
  • You may want to skip the tests found in 4c115c9 on pypy. When we do, we should likely open an issue in the scikit-image repo too.

@stefanv
Copy link
Contributor

stefanv commented Mar 29, 2023

That's a decent set of passes. Now, most of the remaining ones are of the form:

skimage/meson.build:33:0: ERROR: Command `$PREFIX/bin/python -c 'import os; os.chdir(".."); import numpy; print(numpy.get_include())'` failed with status 1.

Still no aarch64 / arm64 builds.

@stefanv
Copy link
Contributor

stefanv commented Mar 29, 2023

I don't think there's much more I can do to help here. I'll follow along, just in case some knowledge of the build system is helpful.

@hmaarrfk We don't cross-compile for macosx, we just build natively: https://github.com/scikit-image/scikit-image/blob/main/.github/workflows/wheel_tests_and_release.yml

The one tricky bit with that build was a bug in meson-python where they look at MACOS_DEPLOYMENT_TARGET instead of at MACOSX_DEPLOYMENT_TARGET.

@stefanv
Copy link
Contributor

stefanv commented Mar 29, 2023

I thought scipy had already trailblazed meson + scientific python? is that not true? I thought we were following the build procedure of an other major package? Can we use their recipe as a model?

Not that I could find at https://github.com/conda-forge/scipy-feedstock or https://github.com/conda-forge/scipy-feedstock/pulls

@stefanv
Copy link
Contributor

stefanv commented Mar 29, 2023

I thought scipy had already trailblazed meson + scientific python? is that not true? I thought we were following the build procedure of an other major package? Can we use their recipe as a model?

Not that I could find at https://github.com/conda-forge/scipy-feedstock or https://github.com/conda-forge/scipy-feedstock/pulls

OK, I found the work that SciPy's done that can (maybe?) get us our macosx cross-compiled binaries!

conda-forge/scipy-feedstock#205

@hmaarrfk
Copy link
Contributor

Still no aarch64 / arm64 builds.

See builds in
#101

aarch seems to run and pass tests on linux.

@stefanv
Copy link
Contributor

stefanv commented Mar 29, 2023

@isuruf I apologize for tagging you personally, but I think we are quite close on this build here, and wondered whether you had any insight on how to get the arm64 cross-compilation going?

@isuruf
Copy link
Member

isuruf commented Mar 29, 2023

#101 seems to be going in the correct direction

@k-dominik
Copy link
Contributor Author

closed in favor of #101

@k-dominik k-dominik closed this Mar 29, 2023
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.

6 participants