-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
MAINT Clean-up more unused Makefile targets #29296
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
Conversation
|
||
clean: | ||
$(PYTHON) setup.py clean | ||
rm -rf dist | ||
|
||
in: inplace # just a shortcut |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be nice to replace this in
target with what we do now though. As in, still be able to do make in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be a bit weird that in
is not the same as inplace
anymore though ... more generally see #29296 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather rename this to dev-setuptools
for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we could do this, although dev-setuptools
would be different to dev-meson
in the sense that you need make dev-setuptools
each time you change a Cython file.
Keeping make inplace-setuptools
you could argue that if some people are still using make in
, they can still make in<TAB>
so slightly less disruptive. But the number of these people is beween 0 and 1 apparently so 🤷.
I am hoping to look at #29012 again so in the medium term it does not matter that much, so I am fine either way ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also fine either way, we should not bother too much since we expect to have less and less people installing using setuptools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for dev-setuptools
. Otherwise it's too confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dev-setuptools it is, I also reordered the targets in rough order of importance
Can we then have a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if CI is green.
Seriously now this is the HTTP 403 in |
FWIW, on the |
I don't want to hear about any more weird problems, I am trying to work here please 😅. Actually right now, I am feeling like a headless chicken running around randomly and bouncing between different red CIs 🐔 😵💫 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but we should wait for @jeremiedbb's opinion before merging this as he is the last maintainer to still use this AFAIK ;)
|
||
clean: | ||
$(PYTHON) setup.py clean | ||
rm -rf dist | ||
|
||
in: inplace # just a shortcut |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather rename this to dev-setuptools
for consistency.
Makefile
Outdated
clean-meson: | ||
pip uninstall -y scikit-learn | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not keeping this one ? Also, is it enough ? You don't need a git clean
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, since I need to run the commands manually on windows, I find it nice to just have to look into the Makefile to find them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can keep it if you want, basically you need this if you switch between setuptools and Meson.
If you only use Meson, you do once make dev-meson
and then importing sklearn
will recompile as needed at import-time. That means, you never have to think do I need to build or not.
Also I find that since scikit-learn is installed as a package that if I want to remove it, pip uninstall scikit-learn -y
is something that most people know how to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With Meson (or rather ninja) in principle you never need to clean. In the case I wanted to make 100% sure there was not something really fishy happening, I did rm -rf build
because that's where pip install -e .
will put stuff by default something like build/cp312
for a Python 3.12 install.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this is broken (using meson-python 0.16.0):
$ make dev
...
$ rm -rf build # alternatively: git clean -xdf
$ python -c "import sklearn; sklearn.show_versions()"
Traceback (most recent call last):
File "<string>", line 1, in <module>
File "<frozen importlib._bootstrap>", line 1176, in _find_and_load
File "<frozen importlib._bootstrap>", line 1138, in _find_and_load_unlocked
File "<frozen importlib._bootstrap>", line 1078, in _find_spec
File "/Users/ogrisel/miniforge3/envs/dev/lib/python3.11/site-packages/_scikit_learn_editable_loader.py", line 310, in find_spec
tree = self._rebuild()
^^^^^^^^^^^^^^^
File "/Users/ogrisel/miniforge3/envs/dev/lib/python3.11/site-packages/_scikit_learn_editable_loader.py", line 338, in _rebuild
if self._work_to_do(env):
^^^^^^^^^^^^^^^^^^^^^
File "/Users/ogrisel/miniforge3/envs/dev/lib/python3.11/site-packages/_scikit_learn_editable_loader.py", line 325, in _work_to_do
p = subprocess.run(dry_run_build_cmd, cwd=self._build_path, env=env, capture_output=True)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/ogrisel/miniforge3/envs/dev/lib/python3.11/subprocess.py", line 548, in run
with Popen(*popenargs, **kwargs) as process:
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/ogrisel/miniforge3/envs/dev/lib/python3.11/subprocess.py", line 1026, in __init__
self._execute_child(args, executable, preexec_fn, close_fds,
File "/Users/ogrisel/miniforge3/envs/dev/lib/python3.11/subprocess.py", line 1950, in _execute_child
raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: '/Users/ogrisel/code/scikit-learn/build/cp311'
Ideally one would want meson-python to tolerate that and rebuild the missing build directory, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I probably was not clear in my previous comment before, here is what I wanted to say:
- if you are only working with Meson, you do
make dev-meson
once and then use scikit-
learn since importing will retrigger compilation. I guess people who have switched to Meson probably never had to think much about "cleaning", it just works pip uninstall -y scikit-learn
is the way to "clean" Meson, in case you want to switch to setuptools. I am guessing most people don't have to do this unless you switch to an old PR.- every once in a while, like every month, I am looking at a fishy behaviour, and just because I am paranoid I do
rm -rf build
. This is clearly not needed and also not a good idea to do in general because you may have other build there of scikit-learn built for other other environments and Python versions.
All in all if you insist you want a make clean
and make clean-meson
I would stick with the previous behaviour of only doing pip uninstall -y scikit-learn
If you want an example of a fishy behaviour, I was looking why somehow we lost pytest assertion rewriting in Meson (i.e. pytest is not showing the value of values on each side of the assertion see #29253 (comment)), so I was switching between Meson and setuptools, and I did not want to take any risks so I did rm -rf build
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's helpful to have an official way to handle the cleaning of a dev build that does not result in one of the broken states I posted above.
Because indeed some people are paranoid and will attempt to do a "clean" operation manually from time to time (even if it's useless, just in case) and end up in the same state as I did.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because indeed some people are paranoid and will attempt to do a "clean" operation manually from time to time
I'm one of those guys and I'm pretty sure it got me out of a very broken state sometimes :)
Anyway, even if it trivial, I'd rather have the shortcut, and I like being able to see all the necessary commands explicitly at one place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK is my last commit good enough or you want an additional clean-paranoid
target that does rm -rf build
(on top of clean
that does pip uninstall -y scikit-learn
😉)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good enough for me
well I just run |
once is better than never 😉 |
0a624ea
to
6e48562
Compare
Auto-merging my own PR with 3 approvals, I looked at all the comments, and I am reasonably confident they have been addressed. If I missed something we can tackle this in a follow-up PR. Summary:
|
Follow-up of #29170 and preparing for #29012.
In rough order of controversy potential, from from 1 to 10 (from "no brainer"=1 to "super controversial"=10)
make cython
controversy rating 1: never heard of it, not sure what it does, probably nobody uses itmake test-*
removal controvery rating 1: and similar have been broken for a few years. In particular since the global random seed plugin, you need to install scikit-learn in your environment or usepython -m pytest
that adds current working directory to sys.path automatically, nobody has ever complained about this it seems until Unable to run code tests, both serial and parallel #29280 so I am guessing this is not usedmake doc
,make doc-noplot
removal controversy rating 2: undocumented so people go todoc
and domake html
ormake html-noplot
all
removal controversy rating 3: I have a slight preference that people be explicit about what they want to do ... theMakefile
will go away one day we use spin ENH Add minimal support for spin as a developer tool #29012. If there is a strong support for having the same behaviour as previously (build + test) I can put it back.Other changes after @adrin's suggestion with higher controversy rating:
dev
fordev-meson
to have something less generic, controversy rating 3clean-meson
rather thanclean
,inplace-setuptools
rather thaninplace