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

ENH Adopt spin as a developer tool #29012

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

lesteve
Copy link
Member

@lesteve lesteve commented May 14, 2024

I mentioned this in the January developer meeting, see meeting notes for more details about motivations.

Right now this is WIP in particular:

  • the list of commands to make available is a bit arbitrary and some of it is copied and pasted from a spin example
  • the developer doc was not updated

Feed-back more than welcome!

Main advantages of switching to spin

You get some nice output when typing spin that makes things more discoverable (rather than having to remember a long-winded command and finding it in the shell history)

image

A few nice things you can do easily

  • build in editable mode: spin install --editable -v
  • run tests: spin test. Because we are using editable you can still use pytest directly if you still want, but I guess having a uniform interface for newcomers may be an improvement. One caveat is that you need to add -- if you want to pass additional pytest arguments e.g. spin test -- sklearn/tests/test_isotonic.py rather than pytest sklearn/tests/test_isotonic.py
  • build doc: spin docs html, build doc without running examples spin docs html-noplot

Try it out locally

You need to install spin 0.9 (only available through pip and not conda at the time of writing but this will be fixed soon see conda-forge/spin-feedstock#7) that has support for Meson editable install.

See https://github.com/scientific-python/spin for more details about spin

Possible improvements

  • using verbose in Meson editable install (to have some output when things are recompiling on sklearn import) does not seem straightforward but there is likely a way to do it with a spin custom command
  • spin docs makes choices about sphinx options like warnings as errors that we may not want to do. Maybe this is overridable, needs a closer look

Any feed-back more than welcome!

Concerns raised during the January developer meeting

newcomers need to learn/understand/trust yet a new tool

I think this is certainly a price to pay but to me this is definitely worth since you don't have to remember (or be able to find in the shell history or in the online doc) arcane long-winded command. Experienced contributors are used to this but for newcomers this is definitely a stumbling block.

does it have a verbose mode where commands are printed to the screen?

In my short experience, it indeed by default outputs information about the commands being run. If we use custom commands, adding this kind of helpful output will be our responsibility see scientific-python/spin#161 where I asked about this.

need to work on avoiding a layering of tools, e.g. spin calls make calls setuptools calls pytest and so on - a layer cake of tools that call tools

So no good answer on this one ... this kind of happens already since spin docs html goes through the Makefile. Maybe there are some opportunities to improve the situation in spin itself. See scientific-python/spin#161 (already mentioned above) where I asked about this as well.

Copy link

github-actions bot commented May 14, 2024

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 5510d99. Link to the linter CI: here

@stefanv
Copy link
Contributor

stefanv commented May 14, 2024

From the spin side, we are very willing to adapt the tool to best address the projects' use-cases; it is meant to make development easier, not to obfuscate things further!

Perhaps the simplest way to think of it, in context of this project, is that it is a Python tool that replaces the Makefile. So, no more of a "layer cake of tools" than you already have 😅 Because it is a Python tool, however, we can improve the user interface: print a decent help, add flags, etc. Because it is not written in Make, you can put logic in your commands that other developers can understand (spin was written from the outset to allow customization; we provide built-in commands, to make it easier to get going, but we always knew project needs would differ). In the sense of the broader ecosystem, it is convenient to be able to use the same user interface to build numpy, scikit-learn, and several other projects (it started when projects switched to Meson, because suddenly seasoned developers didn't know which command to invoke; and, of course, this could happen in other areas too, such as switching docs from sphinx to mystmd, etc.).

As gladly as we'd support scikit-learn's use cases, the project should use what works best for its purposes; and if that's Make, that's perfectly fine too :)

Let me know if you have any questions on using/customizing/extending the tool.

@stefanv
Copy link
Contributor

stefanv commented May 14, 2024

Looking at the existing Makefile, I think you can aggregate targets into the following spin commands (with flags, to select behaviors)

build
test
docs
lint
tags

@adrinjalali
Copy link
Member

One issue I have with spin while working on Numpy is that it doesn't allow for editable installs. Or at least on numpy side, one needs to do either spin build OR pip install -e .... and cannot mix the two.

@lesteve
Copy link
Member Author

lesteve commented May 14, 2024

One issue I have with spin while working on Numpy is that it doesn't allow for editable installs.

This is actually fixed in spin 0.9, released 2 weeks ago, which prompted me to take another look at spin. This is mentioned in my too long PR description, I did not have/take the time to make it shorter 😅

See scientific-python/spin#155 if you are curious about the details. AFAICS this is mostly:

  • having a way to detect if package is installed in editable mode
  • if installed in non-editable mode set PYTHONPATH when running the tests/python code. if installed in editable mode nothing to do.

@adrinjalali
Copy link
Member

@lesteve I saw you mentioning editable install in the OP, but I figured this must be only for "easy" packages, otherwise numpy wouldn't say spin can't do editable installs. But if it works, then that seems nice.

Note that latest spin on conda-forge seems to be 0.8 though.

@lesteve
Copy link
Member Author

lesteve commented May 14, 2024

But if it works, then that seems nice.

It works for scikit-learn in this PR (at least in my quick tests), I am pretty sure the spin PR adding support for editable install was tested in scikit-image. I don't see why it would not work for numpy. Having said that, there may be some limitations that will be discovered when more people try to use spin with editable install. It's hard to make predictions especially about the future as the saying goes 😉

Note that latest spin on conda-forge seems to be 0.8 though.

Yeah I know mentioned in my too long PR description too 😉

pyproject.toml Outdated
# "spin.cmds.meson.test"
# ]
# "Environments" = [
# "spin.cmds.meson.ipython",
Copy link
Member

Choose a reason for hiding this comment

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

on numpy, when working with spin, one cannot run python/ipython/pytest manually. So it's not that it's a shortcut, it's more like that this is the only entry point. I'm okay with that, but then we need these commands here too.

Copy link
Contributor

@stefanv stefanv May 14, 2024

Choose a reason for hiding this comment

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

If you build with meson into, say, build-install, you cannot run python/ipython/pytest without setting some paths. Spin does that for you, unless you are using an editable install (which you can also get with meson, via pip install -e --no-build-isolation).

I.e., it's not because of spin that you cannot access other entry-points, but because of the build/install mechanism used. Installing into ./build-install is the default behavior for spin build on NumPy (and most meson-based projects).

There are lots of moving pieces, so I'm sorry if this is coming across overly pedantic; I just want to clarify what's happening, and help figure out what best workflow defaults are for sklearn.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for clarifying this. From the perspective of the users/contributors, it doesn't matter what the tool does. When users run a command which they expect to install the package, then it should behave as if it's installed.

So in this case, we might want to make sure spin build actually installs the package in the environment, at least for scikit-learn.

Copy link
Contributor

@stefanv stefanv May 14, 2024

Choose a reason for hiding this comment

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

Perhaps it's worth splitting the users and contributors. Users would install via pip or conda, typically, whereas developers want a working build that gets updated when they modify the code. The advantage of editable installs come especially when testing with other packages (i.e., you can run another project's test suite, and it will pick up your dev package). But if you're happy to have an editable install (which retriggers builds and differs in some other ways from a regular package install), then you can set build to the spin.cmds.pip.install built-in command.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry I had local modifications that I did not push which now folded this discussion oh well ...

I think the best way forward on this would be to have spin build-editable exist and probably not spin build to avoid that people mix both, i.e. with spin you can choose which commands you make available. We would also need the Meson verbose editable install so we kind of need a custom command I think.

Now I "just" need to find a good example of custom command to put some kind of reality behind the words 😉

Copy link
Member Author

@lesteve lesteve May 14, 2024

Choose a reason for hiding this comment

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

That would be super useful thanks!

I guess the simplest thing I can think of is I would like spin build-editable (build-editable naming could be improved) to do

pip install --editable . \
    --verbose --no-build-isolation \
    --config-settings editable-verbose=true

Bonus points:

  • reusing the spin.cmds.pip.install and appending the --config-settings to it would be nice
  • more generally it may be nice to have spin build --editable to build in editable mode although not high priority I think install and build are quite different so let's forget about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. What does "build in editable mode" mean in this context?

Copy link
Contributor

Choose a reason for hiding this comment

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

See scientific-python/spin#192 ; with that in place, we can use the command as-is, and just set --verbose as the default option.

Copy link
Contributor

Choose a reason for hiding this comment

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

Examples of how NumPy overrides spin commands, as well as implement their own: https://github.com/numpy/numpy/blob/main/.spin/cmds.py

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it. What does "build in editable mode" mean in this context?

Yeah not great wording, I think it is more "install in editable mode" indeed

@stefanv
Copy link
Contributor

stefanv commented May 14, 2024

I just checked, and spin seems to work fine with NumPy+editable installs. You probably saw the recommendation in their doc/source/building/index.rst, but that notification was written before this feature landed. I've been waiting before some more testing before changing the "official" advice.

@glemaitre
Copy link
Member

While I was more on the -1 side a couple of months ago I since used stuff like pixi and I can clearly see the benefit of having a cross-platform tool. Today, I would genuinely change my mind and go with such a tool.

A separate question unrelated to this PR since @stefanv is around :). I played with pixi in this toy project and I'm wondering if spin would envision something related to environment management. I would see to potential benefit: (i) it allows contributors to start with a working environment known to be working and (ii) the multi-environment can be used for the CIs (as with have our own lock files now) and even with the setup-pixi GHA (https://github.com/prefix-dev/setup-pixi).

@betatim
Copy link
Member

betatim commented May 16, 2024

Like Guillaume I am one of the people not super in favour of having tools that are "too powerful" provide this abstraction. "too powerful" is of course ambiguous and undefinable - my main point of worry is that using a tool like a Makefile, dev containers, spin, etc allows you to hide complexity. And then layer some more complexity on top (e.g. a Makefile inside a dev container). IMHO the fix for your "build and install my project in editable mode" command being too complicated to type/remember is not to add a layer of abstraction, but instead to work on making it possible to have a simpler command.

However, I am softening to the idea of a tool like spin. Call it a +0. I think if we do adopt it, we should remove some of the other "spin like" tools currently in use (maybe that is just the Makefile?). Just to keep the combinatorial jungle in check, avoiding the potential for "layers on layers" and having to maintain several solutions. I am poorly informed about what "spin like" tools are currently in use, because I type pytest --my-favourite-option --just-saw-on-stackoverflow when I want to run the tests.

My final point: I care about being able to mix and match a tool like spin and "direct invocation" (aka pytest -k "array_api"). The reason I care is partly because I like typing things into my terminal ;) but also because I've spent too much time of my life trying to work out what that custom ./build.sh --clean run-tests -- -k array_api actually does and why it is failing. This includes things like "layers upon layers", looking inside it and finding a 15 line cmake command with 25 flags, etc.

pyproject.toml Outdated
Comment on lines 229 to 234
"Environments" = [
"spin.cmds.meson.shell",
"spin.cmds.meson.ipython",
"spin.cmds.meson.python",
"spin.cmds.meson.run"
]
Copy link
Member

Choose a reason for hiding this comment

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

Do we need these? I'm asking because right now I can install in editable mode and then type ipython or ipython -i some_script.py or what ever weird flags I'd like to pass to ipython today and it just works. Asked differently: what problem is solved by providing these?

Copy link
Member Author

@lesteve lesteve May 16, 2024

Choose a reason for hiding this comment

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

Indeed I copied and pasted this part from an example and I think can be removed. These commands mostly makes sense for non-editable workflow (with spin build you have an out-of-tree Meson build and you set PYTHONPATH to be able to import sklearn)

Copy link
Member

Choose a reason for hiding this comment

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

If we don't want to use the "out of tree" feature of meson then I agree we can remove it. Is there ever a use-case for doing a "not editable install" while doing development work?

@stefanv
Copy link
Contributor

stefanv commented May 16, 2024

A separate question unrelated to this PR since @stefanv is around :). I played with pixi in this toy project and I'm wondering if spin would envision something related to environment management. I would see to potential benefit: (i) it allows contributors to start with a working environment known to be working and (ii) the multi-environment can be used for the CIs (as with have our own lock files now) and even with the setup-pixi GHA (https://github.com/prefix-dev/setup-pixi).

Our aim so far has been to keep spin as simple as possible. I.e., limited dependencies, and strictly a more user-friendly replacement for the Makefile approach sklearn uses. We also do not currently have any mechanism for storing options (everything is specific via flags on the prompt). I.e., we can't yet "remember" state, such as the current environment.

We could consider adding some lightweight env management, but why not use nox or one of the pixi tools you mention?

@stefanv
Copy link
Contributor

stefanv commented May 16, 2024

my main point of worry is that using a tool like a Makefile, dev containers, spin, etc allows you to hide complexity.

This is my main worry too :) As such, spin (at least for the internal commands we provide) always prints the commands it executes to the terminal.

And then layer some more complexity on top (e.g. a Makefile inside a dev container). IMHO the fix for your "build and install my project in editable mode" command being too complicated to type/remember is not to add a layer of abstraction, but instead to work on making it possible to have a simpler command.

We don't always have control over the command lines of the tools we used, however, so it'd be hard to get all tool developers to provide each project with exactly what it needs so that arcane flags are no longer necessary. The proof of this lies in the existence of Makefiles in numerous projects.

I think a cool example of this just came out of this conversation: scientific-python/spin#192 I wasn't aware of that meson feature!

However, I am softening to the idea of a tool like spin. Call it a +0.

I'll take it 😂 No, seriously, I discourage people from adopting the tool unless it makes obvious sense. Development should be kept simple; we don't need additional mental overhead. If simple means adopting spin, then great, but there's obviously more than one way to do it.

I think if we do adopt it, we should remove some of the other "spin like" tools currently in use (maybe that is just the Makefile?).

I would not keep both around; "there should be one obvious way" and all that.

My final point: I care about being able to mix and match a tool like spin and "direct invocation" (aka pytest -k "array_api").

Another concern we share. We had this problem with non-in-place meson builds (until recently, those didn't exist!) where we couldn't easily launch commands and have them pick up the package. So spin run pytest -k "array_api" was used to set the PYTHONPATH and then execute the command.

In scikit-learn's case, it seems like most developers are happy to use the editable install route exclusively, in which case many of the meson commands are no longer necessary.
When developing, say, NumPy and SciPy, I tend to have a stable NumPy install in my working env (which SciPy picks up), but then I also want to modify NumPy and test my changes in that library. For that use-case, the meson build-install approach is useful, even though I could have solved it with multiple envs as well.

@glemaitre
Copy link
Member

We could consider adding some lightweight env management, but why not use nox or one of the pixi tools you mention?

Basically, while spin and pixi are initiated to solve different problem, one as a "task manager" and the other one as an "environment manager", I'm under the impression that they could eventually meet somewhere.

Basically, I used pixi to define tasks that are equivalent to the spin doc, spin test, etc. So using pixi only for the environment would be a bit wasteful considering what it offers. However, those tasks have to be redefined per project and this is where I see the value of spin because we end-up having a large overlap between all scientific packages.

I kind of already mentioned this point to @wolfv at the last PyConDE but my view might be not reasonable at all :)

@stefanv
Copy link
Contributor

stefanv commented May 16, 2024

I kind of already mentioned this point to @wolfv at the last PyConDE but my view might be not reasonable at all :)

It may be worth having a chat. No need to build the same thing twice, but the pre-packaged commands (or the logic of them) may still be worth shipping, even if we use alternative scaffolding.

@adrinjalali
Copy link
Member

Seems like pixi should allow plugins and our stack writes a plugin like spin for it 😁

@wolfv
Copy link

wolfv commented May 18, 2024

Thanks for the ping :) @stefanv - would love to chat anytime. I am currently at PyCon US (you too?). Otherwise happy to jump on a call and discuss. Spin looks cool!

@Charlie-XIAO
Copy link
Contributor

Is it possible to make things also work for Windows? Currently spin docs html does not work for me because Makefile does not work on Windows (there is the make.bat instead).

@stefanv
Copy link
Contributor

stefanv commented May 21, 2024

Is it possible to make things also work for Windows? Currently spin docs html does not work for me because Makefile does not work on Windows (there is the make.bat instead).

Spin is tested on Windows. You can have spin invoke a platform platform-specific Makefile, although I already test docs building on Windows using git-bash.

@wolfv
Copy link

wolfv commented May 21, 2024

that would be one benefit of pixi - pixi tasks are designed to run on Windows and Unix with the same syntax :)

@stefanv
Copy link
Contributor

stefanv commented May 21, 2024

that would be one benefit of pixi - pixi tasks are designed to run on Windows and Unix with the same syntax :)

I don't think pixi can fix this problem: it's the underlying Sphinx Make file that's not working on Windows.

@lesteve
Copy link
Member Author

lesteve commented May 21, 2024

Not sure it is worth it, but I guess it seems feasible that spin uses make.bat on Windows if it exists (I thought on Windows make is supposed to find make.bat but maybe it does not work in this case because this is run through subprocess not sure at all ...). IIRC sphinx-quickstart generates both Makefile and make.bat.

I quickly checked and at least numpy and matplotlib still have a make.bat for their docs. Maybe most Scientific Python projects have dropped make.bat and advise contributors to use git bash on Windows, WSL, or something else ...

@stefanv
Copy link
Contributor

stefanv commented May 21, 2024

Maybe most Scientific Python projects have dropped make.bat and advise contributors to use git bash to Windows, WSL, or something else ...

My sense is that that's the general trend, since you have trouble building the library on a "traditional" Windows shell. But spin is not opinionated about this sort of thing...

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.

None yet

7 participants