Update `spin` to make it a drop in replacement of `scipy/dev.py` #238

czgdp1807 · 2024-10-07T17:20:07Z

Regarding context - We are shifting to spin for building, testing SciPy. This PR is for updating spin to make it work with the SciPy building process. One explanation for the update is given below. That is significant. Rest for the smaller updates in spin I will provide explanation in the code review when I will complete the work.

In SciPy each step i.e., compile, setup and install needs arguments from meson_args. You can't pass everything from meson_args tuple directly into the first command. So I added 3 keys, "compile", "setup" and "install" to differentiate which meson argument from SciPy to pass in which step.

Relevant PR from SciPy - scipy/scipy#21674
~~Relevant PR from NumPy - numpy/numpy#27606~~

rgommers · 2024-10-23T08:36:15Z

This does look like a useful improvement, thanks for working on it @czgdp1807. From the code I think I deduce (but didn't verify) that the changes aren't backwards compatible - is that right? If so, is it possible to just add the extra arguments at the end to preserve backwards compat?

czgdp1807 · 2024-10-23T10:01:14Z

From the code I think I deduce (but didn't verify) that the changes aren't backwards compatible - is that right?

Yes. I will make it backwards compatible by tonight. I updated NumPy instead but that isn't the right approach because there are users of spin outside NumPy, SciPy. Closed the NumPy PR.

czgdp1807 · 2024-10-23T10:09:12Z

spin/cmds/meson.py


-    setup_cmd = _meson_cli() + ["setup", build_dir, "--prefix=/usr"] + meson_args
+    setup_cmd = _meson_cli() + ["setup", abs_build_dir, "--prefix={}".format(abs_install_dir)] + meson_args_setup


@ngoldbaum You might be interested in this change since you are working on https://github.com/scientific-python/spin/pull/241/files. Here I have generalised the value of prefix because SciPy needed this. dev.py (in SciPy) provides full path for prefix. In fact, now we won't be depending on checking the OS. Let me know if its helpful. If so, then I will send this change in a separate PR.

I think I'd prefer approach in my PR where it's possible to explicitly set prefix, I don't think that's possible here.

I'd also really like it if it were possible for downstream projects to adapt to changes in commands without explicitly updating their own spin code.

Makes sense to me. Basically the changes I made work for both SciPy and NumPy as well. The changes in main branch of spin work only with NumPy and not with SciPy. Your change is better (it is backwards compatible), however since --prefix=/usr doesn't work with SciPy, so we will have to pass --prefix=<abs_install_dir> in SciPy project.

Basically the choice to be made here is,

Option 1 - Follow https://github.com/scientific-python/spin/pull/241/files and pass --prefix=<abs_build_dir>.

Option 2 - Follow this PR (once it's merged in main branch of spin). Then NumPy and SciPy will work fine as is. However, its hard to say if other projects using spin will break or not.

I am fine with Option 1. That way only SciPy interface will need changes and no other project using spin will break.

cc: @rgommers @stefanv

SciPy has the same constraints as NumPy. It doesn't matter what --prefix is set to, I'd expect things to work for both project. The actual install location is controlled by --destdir; --prefix is turned into a relative path under destdir. Look in numpy/build-install/ to see what I mean (the next subdir there is usr).

Another --prefix patch landed in main already. Did you try dropping this change @czgdp1807?

spin/cmds/meson.py

ngoldbaum · 2024-10-23T10:19:24Z

I updated NumPy instead but that isn't the right approach because there are users of spin outside NumPy, SciPy. Closed the NumPy PR.

I don't think NumPy can use this without updating NumPy's spin config to match. That at least was my experience adding a prefix argument, thus my opening #242

czgdp1807 · 2024-10-23T10:21:21Z

One question to ask - What would be the right behaviour in case of NumPy? spin build and spin test should work perfectly? And import numpy should work without any issues?

ngoldbaum · 2024-10-23T10:29:34Z

Sorry, I don't quite understand what you're asking.

Upstream spin changes shouldn't break downstream projects, no.

Ideally also (IMO) projects should be able to adopt changes to commands defined in spin itself by simply updating spin. Right now at least NumPy has to update spin, then update any commands NumPy overrides to get the new feature. Already NumPy's spin build is out of date and can't use the gcov argument that's defined in the upstream command. The docstring for numpy's spin build is also out of date.

This is a shame because all NumPy really wants to do is add a single argument to spin build. IMO it should be possible to do that without updating the signature or docstring of NumPy's build function.

czgdp1807 · 2024-10-23T10:32:03Z

I see. Let me reword my question. I just want to check whether my changes here break anything in NumPy's build process. If spin build and spin test work fine then can I assume that my changes haven't broken anything in NumPy.

rgommers · 2024-10-23T11:01:24Z

Let me reword my question. I just want to check whether my changes here break anything in NumPy's build process. If spin build and spin test work fine then can I assume that my changes haven't broken anything in NumPy.

Yes, that should be the case. After this PR is updated to remove backwards-incompatible changes, it should also be straightforward for the code reviewers here to see that nothing could have broken. All you want is a new way to pass on extra flags to meson compile and meson install; if numpy doesn't yet use that new feature, nothing should be breaking.

czgdp1807 · 2024-10-24T14:16:42Z

spin build and spin test works perfectly in NumPy with this PR. So I think nothing breaks in NumPy because of the updates I made in this PR.

czgdp1807 · 2024-11-01T16:08:41Z

Update - The support for spin bench is complete in SciPy and in spin (here). spin bench and python dev.py bench work in the same manner.

stefanv · 2024-11-01T16:25:41Z

Thank you so much @czgdp1807. I'm sorry that I haven't reviewed yet; please feel free to bug me if you don't hear from me by Tuesday.

czgdp1807 · 2024-11-08T13:30:24Z

So I discussed with @rgommers and he told me that bench shouldn't be here. That's 100% correct. I should have checked how NumPy implemented spin bench and then should have taken things from there. Anyways, now I will make this PR ready for reviews. What I need to do here is the following,

Drop the bench changes from here.
Explain the rest of the changes.

Once these two are done, it will be ready for review.

czgdp1807

So this PR is ready for review and I have explained the changes to the best of my abilities. Note #238 (comment) too other than the below comments.

cc: @stefanv

czgdp1807 · 2024-11-08T13:44:27Z

spin/cmds/meson.py

-        output=(not quiet) and verbose,
-    )
+            build_dir
+        ] + meson_args_install


This also may or may not be backwards compatible. The change made here is removal of --destdir. I think with this the --prefix will be used as installation directory.

Let me know if I am mis-interpreting things.

I think you don't need these changes. What goes wrong if you don't touch this code at all? If the only thing that changes is the install path going from this:

build-install/lib/python3.12/site-packages/scipy/

to:

build-install/usr/lib/python3.12/site-packages/scipy/

then that is fine, that's a completely harmless change.

Actually if we follow Option 1 from #238 (comment) and undo this change then from my last experience, spin test didn't work correctly. Actually I made the changes here to match with the series of commands executed by python dev.py build. Then everything works in SciPy (also with NumPy). I can try undoing this changes and let you know if SciPy still works.

Yes please:) --destdir is the recommended way of installing to some arbitrary location.

Here is the error if I undo my changes,

(scipy-dev) 20:11:53:~/Quansight/scipy % spin test -s stats Invoking `build` prior to running tests: $ meson compile -j 10 -C /Users/czgdp1807/Quansight/scipy/build INFO: autodetecting backend as ninja INFO: calculating backend command to run: /Users/czgdp1807/mambaforge/envs/scipy-dev/bin/ninja -C /Users/czgdp1807/Quansight/scipy/build -j 10 ninja: Entering directory `/Users/czgdp1807/Quansight/scipy/build' [1/1] Generating scipy/generate-version with a custom command $ meson install --only-changed -C build --destdir ../build-install --tags=runtime,python-runtime,tests,devel $ export PYTHONPATH="/Users/czgdp1807/Quansight/scipy/build-install/usr/lib/python3.12/site-packages" $ /Users/czgdp1807/mambaforge/envs/scipy-dev/bin/python3.12 -P -c 'import scipy' $ cd /Users/czgdp1807/Quansight/scipy/build-install/usr/lib/python3.12/site-packages $ /Users/czgdp1807/mambaforge/envs/scipy-dev/bin/python3.12 -P -m pytest --import-mode=importlib --pyargs scipy.stats -m 'not slow' ===================================================== test session starts ====================================================== platform darwin -- Python 3.12.6, pytest-8.3.3, pluggy-1.5.0 rootdir: /Users/czgdp1807/Quansight/scipy configfile: pytest.ini plugins: cov-5.0.0, anyio-4.4.0, hypothesis-6.112.1, timeout-2.3.1, xdist-3.6.1 collected 0 items / 35 errors ============================================================ ERRORS ============================================================ ___________________ ERROR collecting build-install/usr/lib/python3.12/site-packages/scipy/stats/_bws_test.py ___________________ ImportError while importing test module '/Users/czgdp1807/Quansight/scipy/build-install/usr/lib/python3.12/site-packages/scipy/stats/_bws_test.py'. Hint: make sure your test modules/packages have valid Python names. Traceback: scipy/stats/__init__.py:614: in <module> from ._stats_py import * scipy/stats/_stats_py.py:39: in <module> from scipy.spatial import distance_matrix scipy/spatial/__init__.py:116: in <module> from ._geometric_slerp import geometric_slerp scipy/spatial/_geometric_slerp.py:9: in <module> from scipy.spatial.distance import euclidean scipy/spatial/distance.py:121: in <module> from ..special import rel_entr scipy/special/__init__.py:816: in <module> from . import _ufuncs E ImportError: dlopen(/Users/czgdp1807/Quansight/scipy/build-install/usr/lib/python3.12/site-packages/scipy/special/_ufuncs.cpython-312-darwin.so, 0x0002): Library not loaded: /usr/lib/python3.12/site-packages/scipy/special/libsf_error_state.dylib E Referenced from: <C40E1CEC-A9D0-39F7-8466-1ABD39B3EF5D> /Users/czgdp1807/Quansight/scipy/build-install/usr/lib/python3.12/site-packages/scipy/special/_ufuncs.cpython-312-darwin.so E Reason: tried: '/usr/lib/python3.12/site-packages/scipy/special/libsf_error_state.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/usr/lib/python3.12/site-packages/scipy/special/libsf_error_state.dylib' (no such file), '/usr/lib/python3.12/site-packages/scipy/special/libsf_error_state.dylib' (no such file, not in dyld cache)

The shared library bites again:) No worries, I'm pretty sure I can fix that. I'll try to have a look later today.

To help me here, could you point me to a different spin branch you have that allows me to reproduce this problem from a spin test in a clean repo?

Yes. spin_upd_1 (the branch which contains the changes I reverted from here). Link - https://github.com/czgdp1807/spin/tree/spin_upd_1

It's the same issue as is being discussed in numpy/numpy#27371, it has nothing to do with the shared library but with a breaking change in spin for test discovery that happened in a recent release.

E.g., for spin test -s stats:

ImportError while importing test module '/home/rgommers/code/scipy/build-install/usr/lib/python3.12/site-packages/scipy/stats/_page_trend_test.py'. Hint: make sure your test modules/packages have valid Python names. Traceback: scipy/stats/_page_trend_test.py:4: in <module> from ._continuous_distns import norm E ModuleNotFoundError: No module named 'build-install.usr.lib.python3.12.site-packages.scipy.stats._continuous_distns'; 'build-install.usr.lib.python3.12.site-packages.scipy.stats' is not a package ...

And for spin test -s cluster:

ImportError while importing test module '/home/rgommers/code/scipy/build-install/usr/lib/python3.12/site-packages/scipy/cluster/tests/test_hierarchy.py'. Hint: make sure your test modules/packages have valid Python names. Traceback: scipy/cluster/tests/test_hierarchy.py:54: in <module> from . import hierarchy_test_data E ImportError: cannot import name 'hierarchy_test_data' from 'build-install.usr.lib.python3.12.site-packages.scipy.cluster.tests' (unknown location)

Forbidding relative imports in tests is not great. SciPy has quite a few of those.

@czgdp1807 I suggest to change nothing more here and continue with your SciPy PR, implementing new spin commands. That PR should be unaffected by the outcome of this. Rather than trying to change something in this PR, let's wait for the outcome of the discussion in numpy/numpy#27371.

This seems to work smoothly in spin 0.13 now. I think the comment can be resolved, and no more changes should be needed (🤞🏼).

czgdp1807 · 2024-11-08T13:45:00Z

spin/cmds/meson.py

+             build_dir,
+             ] + meson_args_install,
+         output=(not quiet) and verbose,
+     )


This is harmless and backwards compatible change IMO.

czgdp1807 · 2024-11-08T13:45:46Z

spin/cmds/meson.py

+    cmds = [make_cmd, "-j", str(jobs), sphinx_target]
+    if jobs == "auto":
+        cmds = [make_cmd, "-j", sphinx_target]
+
+    _run(cmds, cwd=doc_dir, replace=True)


This is also harmless and backwards compatible IMO. Just adds support for parallel build of docs.

I'm not sure this is backwards compatible, parallel Sphinx builds are breaking regularly (example: scipy/scipy#18300). From the diff I can't quite tell if the default behavior is switched, but the code looks not quite right to me, since if jobs == 'auto' then -j gets added twice (in lines 950 and 952).

It's also unrelated to the topic of this PR, so I suggest removing the changes. We need this PR to get merged and in a release, so we can move on with spin adoption in SciPy - it's important to keep this focused on the issue at hand. If we need other changes unrelated to passing meson compile and meson install, please put them in a separate PR (or more, one per topic would be great unless things are completely trivial).

Sure. No worries. I added this in the beginning because dev.py has parallel building of docs available. I think we should remove it from dev.py also.

rgommers · 2024-11-08T13:56:01Z

Drop the bench changes from here.

👍🏼. Can you edit the PR title as well to reflect this?

stefanv · 2024-11-17T20:46:26Z

spin/cmds/meson.py

+        meson_args_["compile"] = tuple()
+        meson_args_["install"] = tuple()


I don't see these being used below, so do we need the complication of splitting meson_args into three parts?

Hey @stefanv, the reasons are explained in the following Github comments,

DEV: use spin scipy/scipy#21674 (comment)

@rgommers's response - DEV: use spin scipy/scipy#21674 (comment)

Further explanation from me - DEV: use spin scipy/scipy#21674 (comment)

@rgommers's agreement in - DEV: use spin scipy/scipy#21674 (comment)

My final comment - DEV: use spin scipy/scipy#21674 (comment)

DEV: use spin scipy/scipy#21674 (comment)

This back and forth between me and Ralf will give the context behind this change.

Thanks; what I was asking is how the split is used or exposed: I am not at my computer now, but I can take another look. I may have just missed it, but I thought what I saw was that, apart from the first tuple, the others were empty.

This specific change is for backwards compatibility. Right now NumPy (and probably other spin projects) are passing tuple for meson_args. So I check if it's a tuple then I pass all those arguments to meson setup subcommand (the older behaviour before this PR) and keep everything else empty.

OK, thanks for clarifying. I expected to see some changes to the click parameters to allow specifying such separate sets of settings, but I don't?

Basically it's about how meson is being called in spin. If I want to pass a meson argument to meson install, then spin won't do it. All the meson_args are attached to meson setup subcommand.

Let me know if I am wrong here.

Then after updating scipy's spin config to use the new extension mechanism added in #248 all projects using spin and meson will get the feature "for free".

I am not sure if the issue I am solving here is linked to #248. I will go through it and see if it can be used here as well.

Is there a reason other projects would not benefit from being able to specify those three settings separately? If they would, we could add flags to allow their specification?

If not, I think best to split this into three keyword arguments. That would still allow SciPy to do what it needs, but it's less obscure from the spin perspective (the implicit dictionary structure being used now).

Is there a reason other projects would not benefit from being able to specify those three settings separately? If they would, we could add flags to allow their specification?

Yes, being able to pass arguments to meson compile and meson install is potentially useful to any project that uses Meson.

This PR in its current form does allow that. If it's easier/nicer to add 3 new keywords rather than mash it into meson_args, then sure let's do that.

Made the changes as discussed here.

rgommers · 2024-11-27T10:21:22Z

spin/cmds/meson.py

        compile_flags += ["-j", str(jobs)]

    p = _run(
-        _meson_cli() + ["compile"] + compile_flags + ["-C", build_dir],
+        _meson_cli() + ["compile"] + compile_flags + ["-C", abs_build_dir] + meson_compile_args,


An absolute directory is certainly not needed here, -C takes a relpath

czgdp1807 · 2024-11-27T13:25:04Z

I will open up a new PR for keyword change. Rest will be kept here.

czgdp1807 · 2024-11-27T17:31:48Z

Seems like several improvements were made. The only one left was keyword arguments for meson compile and meson install. So I am closing this PR in favour of #256

czgdp1807 · 2024-11-28T10:35:19Z

Context - This PR was opened at a time when spin didn't have features which I required to make it a drop in replacement of scipy/dev.py. However, at the current stage, everything is there except one feature. It is implemented in #256 and waiting for merge.

czgdp1807 mentioned this pull request Oct 7, 2024

DEV: use spin scipy/scipy#21674

Merged

18 tasks

czgdp1807 mentioned this pull request Oct 21, 2024

DEV: Update spin usage in NumPy (prototype) numpy/numpy#27606

Closed

czgdp1807 commented Oct 23, 2024

View reviewed changes

ngoldbaum reviewed Oct 23, 2024

View reviewed changes

spin/cmds/meson.py Outdated Show resolved Hide resolved

czgdp1807 changed the title ~~DEV: Add compile, setup, and install under meson_args~~ DEV: Add compile, setup, and install under meson_args. Added support for bench as well. Nov 1, 2024

czgdp1807 added 7 commits November 8, 2024 19:01

DEV: Add compile, setup, and install under meson_args

39f37ed

DEV: Fixes in paths passed to meson commands

02dd2d4

DEV: Add support for --show-build-log in SciPy

0dda8c4

DEV: Remove special handling of tuple typed meson_args

1bf888e

DEV: Improvise docs support in spin

3a166f3

DEV: Restore installation command

6cc63f0

DEV: Added backwards compatibility support in meson.py

4b6393c

czgdp1807 force-pushed the spin_upd branch from d2f44f3 to 4b6393c Compare November 8, 2024 13:32

DEV: Remove unused imports from spin

54befb3

czgdp1807 commented Nov 8, 2024

View reviewed changes

czgdp1807 changed the title ~~DEV: Add compile, setup, and install under meson_args. Added support for bench as well.~~ DEV: Add compile, setup, and install under meson_args. Nov 8, 2024

DEV: Use cmd instead of repeating

8d2103f

rgommers mentioned this pull request Nov 8, 2024

MAINT: Update to spin 0.12 numpy/numpy#27371

Closed

stefanv force-pushed the main branch from 2bec334 to 3760320 Compare November 12, 2024 01:52

stefanv reviewed Nov 17, 2024

View reviewed changes

DEV: Use keyword arguments for meson compile and meson install

0cfd908

rgommers reviewed Nov 27, 2024

View reviewed changes

czgdp1807 mentioned this pull request Nov 27, 2024

DEV: Update spin to accept arguments of meson compile and meson install #256

Merged

czgdp1807 changed the title ~~DEV: Add compile, setup, and install under meson_args.~~ Update spin to make it a drop in replacement of scipy/dev.py Nov 27, 2024

czgdp1807 marked this pull request as draft November 27, 2024 14:52

czgdp1807 closed this Nov 27, 2024

czgdp1807 deleted the spin_upd branch November 27, 2024 17:31

rgommers mentioned this pull request Dec 5, 2024

ENH: support shared libraries inside packages #257

Closed


		setup_cmd = _meson_cli() + ["setup", build_dir, "--prefix=/usr"] + meson_args
		setup_cmd = _meson_cli() + ["setup", abs_build_dir, "--prefix={}".format(abs_install_dir)] + meson_args_setup

		meson_args_["compile"] = tuple()
		meson_args_["install"] = tuple()

Update spin to make it a drop in replacement of scipy/dev.py #238

Update spin to make it a drop in replacement of scipy/dev.py #238

Uh oh!

Conversation

czgdp1807 commented Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rgommers commented Oct 23, 2024

Uh oh!

czgdp1807 commented Oct 23, 2024

Uh oh!

czgdp1807 Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ngoldbaum commented Oct 23, 2024

Uh oh!

czgdp1807 commented Oct 23, 2024

Uh oh!

ngoldbaum commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

czgdp1807 commented Oct 23, 2024

Uh oh!

rgommers commented Oct 23, 2024

Uh oh!

czgdp1807 commented Oct 24, 2024

Uh oh!

czgdp1807 commented Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stefanv commented Nov 1, 2024

Uh oh!

czgdp1807 commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

czgdp1807 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

czgdp1807 Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rgommers commented Nov 8, 2024

Update `spin` to make it a drop in replacement of `scipy/dev.py` #238

Update `spin` to make it a drop in replacement of `scipy/dev.py` #238

czgdp1807 commented Oct 7, 2024 •

edited

Loading

czgdp1807 Oct 23, 2024 •

edited

Loading

ngoldbaum commented Oct 23, 2024 •

edited

Loading

czgdp1807 commented Nov 1, 2024 •

edited

Loading

czgdp1807 commented Nov 8, 2024 •

edited

Loading

czgdp1807 left a comment •

edited

Loading

czgdp1807 Nov 8, 2024 •

edited

Loading