MAINT Remove setup.py mentions from documentation and comments #29352

lesteve · 2024-06-26T14:05:54Z

Preparing before removing setup.py completely i.e. #29346.

Things I am not so sure about:

Alternative compilers still mentions setup.py not sure what the use case is to be honest, maybe Intel compilers? I am guessing there is a way to adapt it with Meson, one of them being setting CC, CFLAGS, and similar environment variables?
NPY_NO_DEPRECATED_API I certainly did not port this part in Meson, is it actually still needed now that we compile against Numpy 2? Probably more details are available at https://numpy.org/doc/stable/reference/c-api/deprecations.html and https://numpy.org/doc/stable/dev/depending_on_numpy.html. Scipy seems to use it:
https://github.com/scipy/scipy/blob/fffcaa8c58b6909d22063c29e4f9593059a40230/scipy/meson.build#L76-L77
build_command in asv.conf.json, I copied what numpy does, ~~not tested~~ tested by Jérémy it seems to work

github-actions · 2024-06-26T14:07:20Z

✔️ Linting Passed

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

_{Generated for commit: c64f2c3. Link to the linter CI: here}

jeremiedbb · 2024-06-26T14:45:14Z

build_command in asv.conf.json, I copied what numpy does, not tested

I tested and the new command works. It's actually the new default build command since asv 0.6.2 (latest version is 0.6.3).

jeremiedbb · 2024-06-27T10:05:24Z

I'm ok to remove the alternative compilers section since we're remove setuptools support any time soon and this section was mostly about intel compilers support by setuptools. With other build systems one should use the CC, CFLAGS, LDFLAGS, ... env vars as usual.

sklearn/linear_model/_sgd_fast.pyx.tp

sklearn/ensemble/_gradient_boosting.pyx

jeremiedbb · 2024-06-27T12:41:05Z

doc/developers/cython.rst

@@ -97,8 +97,6 @@ Tips for performance

 * Inline methods and function when it makes sense

-* Make sure your Cython compilation units `use NumPy recent C API <https://github.com/scikit-learn/scikit-learn/blob/62a017efa047e9581ae7df8bbaa62cf4c0544ee4/setup.py#L64-L70>`_.


I'd be interested to have more details about what we need/don't need to do about that before deciding to completely remove it and forget about it. ping @jjerphan, you seem familiar with this stuff :)

This is now taken care of natively by setup.py (note that this might need to be ported to meson):

scikit-learn/setup.py

Lines 121 to 128 in 87fa654

# Always use NumPy 1.7 C API for all compiled extensions.

# See: https://numpy.org/doc/stable/reference/c-api/deprecations.html

DEFINE_MACRO_NUMPY_C_API = (

"NPY_NO_DEPRECATED_API",

"NPY_1_7_API_VERSION",

)

for ext in self.extensions:

ext.define_macros.append(DEFINE_MACRO_NUMPY_C_API)

so this comment can be removed. 👍

I think NPY_NO_DEPRECATED_API must be defined everywhere especially if NumPy 2 is used as old C APIs might be deprecated.

Thanks, I think this part has not been ported to Meson, which is likely an oversight ... fixing this is probably doable.

Not 100% sure, but my understanding is that using NPY_NO_DEPRECATED_API + NPY_1_7_API_VERSION makes sure that we don't use any Numpy C API that is deprecated in Numpy version 1.7.

@jjerphan but do you have a suggestion about which version we should pick? Is NPY_1_7_API_VERSION OK? Would using NPY_1_9_API_VERSION be better, which is what scipy is doing see https://github.com/scipy/scipy/blob/fffcaa8c58b6909d22063c29e4f9593059a40230/scipy/meson.build#L76-L77?

In the meantime I have opened #29357 to do the same thing as what setup.py was doing.

I would also pick NPY_1_9_API_VERSION to be consistent with SciPy's choice. 👍

#29357 has been merged, so I guess this PR can be merged too?

Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>

…t-learn#29352) Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>

Remove setup.py mentions from documentation

3c33c4a

github-actions bot added cython module:ensemble module:linear_model module:utils labels Jun 26, 2024

Add TODO

fd44099

lesteve added 2 commits June 26, 2024 17:15

Comment default command

1737afb

Remove Alternative compilers section

7149099

ogrisel approved these changes Jun 27, 2024

View reviewed changes

jeremiedbb reviewed Jun 27, 2024

View reviewed changes

lesteve and others added 2 commits June 27, 2024 15:15

Update sklearn/linear_model/_sgd_fast.pyx.tp

b02feb7

Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>

Update sklearn/ensemble/_gradient_boosting.pyx

c64f2c3

Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>

lesteve mentioned this pull request Jun 27, 2024

BLD Use NPY_NO_DEPRECATED_API in meson.build to mirror setup.py #29357

Merged

jjerphan approved these changes Jun 28, 2024

View reviewed changes

jjerphan merged commit d840f36 into scikit-learn:main Jun 28, 2024
30 checks passed

lesteve deleted the remove-setup-py-from-doc branch June 28, 2024 11:14

jeremiedbb mentioned this pull request Jul 2, 2024

Release 1.5.1 #29382

Merged

11 tasks

snath-xoc pushed a commit to snath-xoc/scikit-learn that referenced this pull request Jul 5, 2024

MAINT Remove setup.py mentions from documentation and comments (sciki…

07a5fe1

…t-learn#29352) Co-authored-by: Jérémie du Boisberranger <jeremie@probabl.ai>

jeremiedbb mentioned this pull request Jul 16, 2024

BENCH Don't use setuptools based build commands #29500

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAINT Remove setup.py mentions from documentation and comments #29352

MAINT Remove setup.py mentions from documentation and comments #29352

lesteve commented Jun 26, 2024 •

edited

Loading

github-actions bot commented Jun 26, 2024 •

edited

Loading

jeremiedbb commented Jun 26, 2024

jeremiedbb commented Jun 27, 2024

jeremiedbb Jun 27, 2024

jjerphan Jun 27, 2024 •

edited

Loading

jjerphan Jun 27, 2024

lesteve Jun 27, 2024

lesteve Jun 27, 2024

jjerphan Jun 27, 2024

lesteve Jun 28, 2024

		@@ -97,8 +97,6 @@ Tips for performance

		* Inline methods and function when it makes sense

		* Make sure your Cython compilation units `use NumPy recent C API <https://github.com/scikit-learn/scikit-learn/blob/62a017efa047e9581ae7df8bbaa62cf4c0544ee4/setup.py#L64-L70>`_.

	# Always use NumPy 1.7 C API for all compiled extensions.
	# See: https://numpy.org/doc/stable/reference/c-api/deprecations.html
	DEFINE_MACRO_NUMPY_C_API = (
	"NPY_NO_DEPRECATED_API",
	"NPY_1_7_API_VERSION",
	)
	for ext in self.extensions:
	ext.define_macros.append(DEFINE_MACRO_NUMPY_C_API)

MAINT Remove setup.py mentions from documentation and comments #29352

MAINT Remove setup.py mentions from documentation and comments #29352

Conversation

lesteve commented Jun 26, 2024 • edited Loading

github-actions bot commented Jun 26, 2024 • edited Loading

✔️ Linting Passed

jeremiedbb commented Jun 26, 2024

jeremiedbb commented Jun 27, 2024

jeremiedbb Jun 27, 2024

Choose a reason for hiding this comment

jjerphan Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

jjerphan Jun 27, 2024

Choose a reason for hiding this comment

lesteve Jun 27, 2024

Choose a reason for hiding this comment

lesteve Jun 27, 2024

Choose a reason for hiding this comment

jjerphan Jun 27, 2024

Choose a reason for hiding this comment

lesteve Jun 28, 2024

Choose a reason for hiding this comment

lesteve commented Jun 26, 2024 •

edited

Loading

github-actions bot commented Jun 26, 2024 •

edited

Loading

jjerphan Jun 27, 2024 •

edited

Loading