Skip to content

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Jun 21, 2024
Merged

MAINT Clean-up more unused Makefile targets #29296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
86e56eb
MAINT Clean-up more unused Makefile targets
lesteve Jun 19, 2024
5631512
[azure parallel] I guess this was used actually ...
lesteve Jun 19, 2024
b00e0cd
[azure parallel] Fix
lesteve Jun 19, 2024
4b246f8
[azure parallel] debug
lesteve Jun 19, 2024
a2c3e40
more tweaks
lesteve Jun 19, 2024
2b8c59b
[azure parallel] Retrigger CI
lesteve Jun 19, 2024
988f0eb
[azure parallel] clean-up
lesteve Jun 19, 2024
24e4c8b
Put back clean
lesteve Jun 20, 2024
ecadb07
dev-setuptools + reordering
lesteve Jun 20, 2024
6e48562
Add help as default rule
lesteve Jun 21, 2024
c51fb25
Tweak help message
lesteve Jun 21, 2024
f082a64
Good old PHONY
lesteve Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 17 additions & 42 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,55 +1,30 @@
# simple makefile to simplify repetitive build env management tasks under posix

# caution: testing won't work on windows, see README

PYTHON ?= python
CYTHON ?= cython
PYTEST ?= pytest

# skip doctests on 32bit python
BITS := $(shell python -c 'import struct; print(8 * struct.calcsize("P"))')
all:
@echo "Please use 'make <target>' where <target> is one of"
@echo " dev build scikit-learn with Meson"
@echo " clean clean scikit-learn Meson build. Very rarely needed,"
@echo " one use case is when switching back to setuptools"
@echo " dev-setuptools build scikit-learn with setuptools (deprecated)"
@echo " clean-setuptools clean scikit-learn setuptools build (deprecated)"

all: clean inplace test
.PHONY: all

clean:
$(PYTHON) setup.py clean
rm -rf dist

in: inplace # just a shortcut
Copy link
Member

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

Copy link
Member Author

@lesteve lesteve Jun 19, 2024 •
edited
Loading

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)

Copy link
Member

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.

Copy link
Member Author

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 ...

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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

inplace:
$(PYTHON) setup.py build_ext -i
dev: dev-meson

dev-meson:
pip install --verbose --no-build-isolation --editable . --check-build-dependencies --config-settings editable-verbose=true

clean: clean-meson

clean-meson:
pip uninstall -y scikit-learn

test-code: in
$(PYTEST) --showlocals -v sklearn --durations=20
test-sphinxext:
$(PYTEST) --showlocals -v doc/sphinxext/
test-doc:
ifeq ($(BITS),64)
$(PYTEST) $(shell find doc -name '*.rst' | sort)
endif
test-code-parallel: in
$(PYTEST) -n auto --showlocals -v sklearn --durations=20

test-coverage:
rm -rf coverage .coverage
$(PYTEST) sklearn --showlocals -v --cov=sklearn --cov-report=html:coverage
test-coverage-parallel:
rm -rf coverage .coverage .coverage.*
$(PYTEST) sklearn -n auto --showlocals -v --cov=sklearn --cov-report=html:coverage

test: test-code test-sphinxext test-doc

cython:
python setup.py build_src

doc: inplace
$(MAKE) -C doc html

doc-noplot: inplace
$(MAKE) -C doc html-noplot
dev-setuptools:
$(PYTHON) setup.py build_ext -i

clean-setuptools:
$(PYTHON) setup.py clean
rm -rf dist
13 changes: 6 additions & 7 deletions build_tools/azure/test_docs.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
#!/bin/bash

set -e
set -ex

if [[ "$DISTRIB" =~ ^conda.* ]]; then
source activate $VIRTUALENV
elif [[ "$DISTRIB" == "ubuntu" || "$DISTRIB" == "pip-free-threaded" ]]; then
source $VIRTUALENV/bin/activate
fi
source build_tools/shared.sh
activate_environment

make test-doc
# XXX: for some unknown reason python -m pytest fails here in the CI, can't
# reproduce locally and not worth spending time on this
pytest $(find doc -name '*.rst' | sort)
2 changes: 1 addition & 1 deletion doc/developers/contributing.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -996,7 +996,7 @@ We expect code coverage of new features to be at least around 90%.
To test code coverage, you need to install the `coverage
<https://pypi.org/project/coverage/>`_ package in addition to `pytest`.

1. Run `make test-coverage`. The output lists for each file the line
1. Run `pytest --cov sklearn /path/to/tests`. The output lists for each file the line
numbers that are not tested.

2. Find a low hanging fruit, looking at which lines are not tested,
Expand Down