search

flatironinstitute / jax-finufft

JAX bindings to the Flatiron Institute Non-uniform Fast Fourier Transform (FINUFFT) library
Apache License 2.0
77 stars 2 forks source link

Replacing CPATH by CMAKE_PREFIX_PATH in README #81

Closed dfm closed 4 months ago

dfm commented 4 months ago

I've found that starting on a fresh system the README instructions that use CPATH don't work for finding conda-installed libraries. (As far as I can tell, CMake doesn't support CPATH at all...) The instructions here work for me!

lgarrison commented 4 months ago

I agree that this does look better! CPATH isn't for CMake, it's for the compilers, but this seems like an improvement.

Did you test on Mac and Linux? I can test the latter if needed.

dfm commented 4 months ago

Thanks! I've tested it on my Mac, but I haven't tested on Linux yet. I found that the CPATH version would have the upstream FINUFFT CMake trying to install FFTW from source.

lgarrison commented 4 months ago

Interestingly, conda seems to be setting the CMAKE_PREFIX_PATH variable; I'm not sure it was doing that before. It looks like cxx-compiler is responsible for this, maybe indirectly, but I can't find any documentation. The upshot is that the conda-supplied FFTW is being found without setting any environment variables. I think it should be safe to tell users to add the CONDA_PREFIX anyway, but we should probably concatenate rather than set (because conda adds other paths).

CMAKE_ARGS is also getting set:

CMAKE_ARGS=-DCMAKE_AR=/mnt/home/lgarrison/miniforge3/envs/jf-gh81/bin/x86_64-conda-linux-gnu-ar -DCMAKE_CXX_COMPILER_AR=/mnt/home/lgarrison/miniforge3/envs/jf-gh81/bin/x86_64-conda-linux-gnu-gcc-ar -DCMAKE_C_COMPILER_AR=/mnt/home/lgarrison/miniforge3/envs/jf-gh81/bin/x86_64-conda-linux-gnu-gcc-ar -DCMAKE_RANLIB=/mnt/home/lgarrison/miniforge3/envs/jf-gh81/bin/x86_64-conda-linux-gnu-ranlib -DCMAKE_CXX_COMPILER_RANLIB=/mnt/home/lgarrison/miniforge3/envs/jf-gh81/bin/x86_64-conda-linux-gnu-gcc-ranlib -DCMAKE_C_COMPILER_RANLIB=/mnt/home/lgarrison/miniforge3/envs/jf-gh81/bin/x86_64-conda-linux-gnu-gcc-ranlib -DCMAKE_LINKER=/mnt/home/lgarrison/miniforge3/envs/jf-gh81/bin/x86_64-conda-linux-gnu-ld -DCMAKE_STRIP=/mnt/home/lgarrison/miniforge3/envs/jf-gh81/bin/x86_64-conda-linux-gnu-strip -DCMAKE_BUILD_TYPE=Release

This suggests we should be concatenating, rather than setting, CMAKE_ARGS in the GPU installation, although scikit-build-core raises a warning about -DCMAKE_BUILD_TYPE=Release:

  2024-04-19 10:13:26,906 - scikit_build_core - WARNING - Unsupported CMAKE_ARGS ignored: -DCMAKE_BUILD_TYPE=Release

We could try filtering this variable ourselves, but that feels messy. Maybe it's best to just leave it.

So... I think I would recommend we keep the changes in this PR, but prepend to the CMAKE_* variables instead of setting them. Does that sound good to you?

dfm commented 4 months ago

Interesting! I'm seeing that now too. I must have tried once without installing cxx-compiler...

Perhaps our default installation instructions shouldn't set anything, but we could add a section like "Troubleshooting" or "Advanced Installation"?

But, either way, I totally agree about prepending!

lgarrison commented 4 months ago

Interesting! I'm seeing that now too. I must have tried once without installing cxx-compiler...

Perhaps our default installation instructions shouldn't set anything, but we could add a section like "Troubleshooting" or "Advanced Installation"?

Yes, if it's working for you now, I agree the default instructions don't need this step!

dfm commented 4 months ago

Digging into this some more, we might want a few more explicit sections in the README about different installation paths (e.g. conda deps, brew deps, Flatiron-specific, etc.). I'll take a stab at that soon!

dfm commented 4 months ago

Superseded by https://github.com/flatironinstitute/jax-finufft/pull/85