Skip to content

newstyle Find_Python, numpy2, and xtensor 0.26#150

Merged
constantinpape merged 14 commits intoDerThorsten:masterfrom
hmaarrfk:fix_builds_windows
May 3, 2025
Merged

newstyle Find_Python, numpy2, and xtensor 0.26#150
constantinpape merged 14 commits intoDerThorsten:masterfrom
hmaarrfk:fix_builds_windows

Conversation

@hmaarrfk
Copy link
Contributor

I'm just trying to see if a few more things can be simplified from the codebase

still very much a draft at this stage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just created a blank file here...

fail-fast: false
matrix:
os: [macos-latest, windows-latest, ubuntu-latest]
python-version: [3.11]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This parameter doesn't exist (anymore???)

-DCMAKE_PREFIX_PATH:PATH="%CONDA_PREFIX%" ^
-DCMAKE_INSTALL_PREFIX:PATH="%CONDA_PREFIX%" ^
-DPython_EXECUTABLE:PATH="%CONDA_PREFIX%\python.exe" ^
-DCMAKE_CXX_FLAGS="/EHsc" ^
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do you specify /EHsc i'm not familiar with windows but can you remove this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

My windows knowledge is also limited... It seems like this is for suppressing some warnings during compilation. Do you think there is a problem in having it here?

@hmaarrfk hmaarrfk force-pushed the fix_builds_windows branch 2 times, most recently from 5f0b113 to d2a9029 Compare March 19, 2025 15:31
@hmaarrfk hmaarrfk force-pushed the fix_builds_windows branch from d2a9029 to 4704811 Compare March 23, 2025 18:10
@hmaarrfk hmaarrfk force-pushed the fix_builds_windows branch 5 times, most recently from fc3b0d6 to 83eb9ad Compare May 3, 2025 02:47
@hmaarrfk hmaarrfk changed the title Try to use newstyle Find_Python throughout newstyle Find_Python, numpy2, and xtensor 0.26 May 3, 2025
@hmaarrfk hmaarrfk marked this pull request as ready for review May 3, 2025 02:48
@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented May 3, 2025

@constantinpape It would be great to get this in so we can start to get numpy2 compatibility

@constantinpape
Copy link
Collaborator

Hi @hmaarrfk and thanks for looking into this!
It looks like we first need to update xtensor in the z5 upstream dependency: (see test error)

 In file included from /home/runner/micromamba/envs/nifty-build-env/include/z5/multiarray/xtensor_access.hxx:7,
                 from /home/runner/work/nifty/nifty/include/nifty/z5/z5.hxx:3,
                 from /home/runner/work/nifty/nifty/src/python/lib/z5/dataset_wrapper.cxx:5:
/home/runner/micromamba/envs/nifty-build-env/include/z5/multiarray/xtensor_util.hxx:3:10: fatal error: xtensor/xarray.hpp: No such file or directory
    3 | #include "xtensor/xarray.hpp"

I will see if I can take care of it now.

@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented May 3, 2025

Do point me to your fix. Curious to learn!

@constantinpape
Copy link
Collaborator

constantinpape commented May 3, 2025

The problem is/was that z5 was still pinned to an earlier xtensor version, using the previous imports. Luckily this was easy to fix, see conda-forge/z5py-feedstock#78 . So here, we just need to wait a bit till the new version is on conda-forge. Maybe should also pin the z5 version to >=2.0.20.

This should now also enable updating z5py to numpy 2 etc.

Copy link
Collaborator

@constantinpape constantinpape left a comment

Choose a reason for hiding this comment

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

The tests pass now and the changes look good from my side.
Should I merge it @hmaarrfk or do you want to change anything else?
I would then also go ahead with a new release, so that we can get this on conda-forge.

@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented May 3, 2025

I think this is great! Just wanted to help close this exploration we started a while back and see if we can remove your numpy 1 pin on micro-Sam!

Let's keep the merging and releasing momentum going ;)

@constantinpape
Copy link
Collaborator

Ah windows actually fails :( I will check it later.

@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented May 3, 2025

I think cmake4 got more strict with path manipulation.

@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented May 3, 2025

hmm, i think you and i found a similar "fix" nearly at the same time. i don't think your fix worked for me locally though.....

With my debug statements, I can see:

-- Python_SITELIB C:\Users\markh\miniforge3\envs\mcam_dev\Lib\site-packages
-- Python_SITELIB C:/Users/markh/miniforge3/envs/mcam_dev/Lib/site-packages

so it seems to me that Python_SITELIB is likely not normalized in the Find_Python package??? that would be the first time i see it...

@constantinpape
Copy link
Collaborator

hmm, i think you and i found a similar "fix" nearly at the same time. i don't think your fix worked for me locally though.....

Yes, my fix also didn't work in the CI.

so it seems to me that Python_SITELIB is likely not normalized in the Find_Python package??? that would be the first time i see it...

I see. Hopefully your last commit addresses this. Let's see :)

@hmaarrfk hmaarrfk force-pushed the fix_builds_windows branch from 6467ad7 to 4a2a254 Compare May 3, 2025 18:46
@hmaarrfk hmaarrfk force-pushed the fix_builds_windows branch from 4a2a254 to e3767b6 Compare May 3, 2025 18:47
@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented May 3, 2025

I see. Hopefully your last commit addresses this. Let's see :)

I was booted into windows up to 10 seconds ago. So I have high hopes.

I spent a bit of time cleaning up commit history. These build problems are annoying but I appreciate your help in unpinning dependencies!

Very excited for this!

@constantinpape
Copy link
Collaborator

Yes, it' fixed now! Thanks for all the efforts :) ! I will go ahead and merge this and create a new release.

@constantinpape constantinpape merged commit df753a6 into DerThorsten:master May 3, 2025
4 checks passed
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.

2 participants