search

conda-forge / scikit-image-feedstock

A conda-smithy repository for scikit-image.
BSD 3-Clause "New" or "Revised" License
4 stars 25 forks source link

update scikit to 0.20.0 #100

Closed k-dominik closed 1 year ago

k-dominik commented 1 year ago

Checklist

conda-forge-webservices[bot] commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@conda-forge-admin, please rerender

jni commented 1 year ago

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

k-dominik commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 https://github.com/scikit-image/scikit-image/pull/6757.

stefanv commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

stefanv commented 1 year ago

@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 commented 1 year ago

Odd: WARNING: The cross-compiler ('arm64-apple-darwin20.0.0-clang') does not appear to be for the correct architecture (got arm64-apple-darwin20.0.0, expected aarch64-apple-darwin20.0.0).

k-dominik commented 1 year ago

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

absolutely appreciate it! :) thank you

hmaarrfk commented 1 year ago

@conda-forge-admin please rerender

hmaarrfk commented 1 year ago

well my fancy pinning for numpy is somewhat a waste given that we just bumped the minimum numpy version https://github.com/conda-forge/conda-forge-pinning-feedstock/pull/4214

but we are are left with osx-arm64 now it seems. I'm not really good at cross compiling with meson yet, so I'm not sure how much I can help quickly.

jni commented 1 year ago

LOL Well I'm still glad to see you pop in here @hmaarrfk! 😂

stefanv commented 1 year ago 
WARNING: The cross-compiler ('arm64-apple-darwin20.0.0-clang') does not appear to be for the correct architecture (got arm64-apple-darwin20.0.0, expected aarch64-apple-darwin20.0.0). Use --cc to correct, if necessary.

I think this error can be ignored. aarch64 and arm64 refer to the same thing.

  ../../meson.build:1:0: ERROR: Could not invoke sanity test executable: [Errno 86] Bad CPU type in executable: '/Users/runner/miniforge3/conda-bld/scikit-image_1679890122540/work/.mesonpy-yujqtg8t/build/meson-private/sanitycheckc.exe'.

Why is it trying to run a .exe now?

Apparently:

sanitycheckc.exe is a native executable, despite the unusual (for Linux at least) file extension.

I would expect this problem to be resolved by:

    - meson-python                           # [build_platform != target_platform]
stefanv commented 1 year ago

Trying host-native meson now. But I think the overlap in those two sections may be problematic, currently. From the docs:

The PREFIX environment variable points to the host prefix. With respect to activation during builds, both the host and build environments are activated. The build prefix is activated before the host prefix so that the host prefix has priority over the build prefix. Executables that don't exist in the host prefix should be found in the build prefix.

stefanv commented 1 year ago

:man_shrugging: Not sure how to get sanitycheckc.exe to be of the correct CPU type.

hmaarrfk commented 1 year ago

@conda-forge-admin please rerender

stefanv commented 1 year ago

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

stefanv commented 1 year ago 
    - clang                                  # [win]

This line does not seem to do what we want; clang is missing during Windows build.

stefanv commented 1 year ago

MacOS reports:

Build type: native build

  ../../meson.build:1:0: ERROR: Could not invoke sanity test executable: [Errno 86] Bad CPU type in executable: '/Users/runner/miniforge3/conda-bld/scikit-image_1679991818027/work/.mesonpy-vnecq_6j/build/meson-private/sanitycheckc.exe'.

That should not be a native build, so Meson isn't being set up correctly.

stefanv commented 1 year ago 
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 commented 1 year ago

@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 commented 1 year ago

@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 https://github.com/conda-forge/scikit-image-feedstock/pull/101

Sorry for the noise here.

hmaarrfk commented 1 year ago

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 commented 1 year ago

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) https://github.com/mesonbuild/meson-python/issues/321

stefanv commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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!

https://github.com/conda-forge/scipy-feedstock/pull/205/

hmaarrfk commented 1 year ago

Still no aarch64 / arm64 builds.

See builds in https://github.com/conda-forge/scikit-image-feedstock/pull/101

aarch seems to run and pass tests on linux.

stefanv commented 1 year ago

@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 commented 1 year ago

101 seems to be going in the correct direction

k-dominik commented 1 year ago

closed in favor of #101