BLD Remove support for setuptools build by lesteve · Pull Request #29400 · scikit-learn/scikit-learn

lesteve · 2024-07-03T12:04:11Z

Close #29346

Fix #29302

Comments:

tolerance was slightly increased in debian 32 CI build, not really clear why this is needed, maybe going from setuptools build to Meson build has different compiler tool flags 🤷
I used a more recent Debian for Debian 32 bit Docker image, otherwise you get an error that the pip does not know how to deal with a pyproject.toml without a setup.py (ah the good old days 🙃)

github-actions · 2024-07-03T12:05:27Z

✔️ Linting Passed

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

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

jeremiedbb · 2024-07-03T13:02:04Z

-def gen_from_templates(templates):
-    """Generate cython files from a list of templates"""
-    # Lazy import because cython is not a runtime dependency.
-    from Cython import Tempita
-
-    for template in templates:
-        outfile = template.replace(".tp", "")
-
-        # if the template is not updated, no need to output the cython file
-        if not (
-            os.path.exists(outfile)
-            and os.stat(template).st_mtime < os.stat(outfile).st_mtime
-        ):
-            with open(template, "r") as f:
-                tmpl = f.read()
-
-            tmpl_ = Tempita.sub(tmpl)
-
-            warn_msg = (
-                "# WARNING: Do not edit this file directly.\n"
-                f"# It is automatically generated from {template!r}.\n"
-                "# Changes must be made there.\n\n"
-            )
-
-            with open(outfile, "w") as f:
-                f.write(warn_msg)
-                f.write(tmpl_)


meson generates the cython files from tempita templates automatically ?
Is there a way to have this warning in the generated files with meson ?

we have a script that has similar logic and is used in meson.build files https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/_build_utils/tempita.py. It was strongly inspired from Numpy and Scipy.

We could have the warning if we really insist but the .c files are out of tree (build/<something> by default) in general they are in some deeply nested subfolder so I would argue it is a lot less tempting to modify them compared to the in-tree situation before?

but the .c files are out of tree

I was more worried about the .pxd and .pyx generated files, but the fact that it's out of tree should prevent any direct edit of these files. So the current situation is fine to me

jeremiedbb · 2024-07-03T13:03:32Z

 """
 Utilities useful during the build.
 """


I guess you can just remove the full _build_utils folder at once since there's nothing left besides this docstring

never mind there are other files...

I removed the docstring but there are stuff that are used e.g. https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/_build_utils/tempita.py https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/_build_utils/version.py. To be honest not sure why they are under sklearn since it feels like maybe they don't have to be? I mirrored Numpy and Scipy here.

Edit: maybe there is a use case when installing from sdist, not sure ...

Yeah you can ignore this comment, I thought you were removing all files and content from this folder, but realized afterward that there are other files left.

Edit: maybe there is a use case when installing from sdist, not sure ...

Not actionable for this PR: I can not think of a good reason for having them under sklearn. _build_utils can be in the sdist at the root level.

Yep I kind of agree ...

lesteve · 2024-07-03T14:25:18Z

The wheels succeeded in 25743a5 see build log.

ogrisel

LGTM once the lock files have been regenerated after a merge with main.

ogrisel · 2024-07-09T14:38:18Z

+        # TODO temporary hack while waiting for version checks inside meson.build
+        # if [[ "$DISTRIB" == "ubuntu" || "$DISTRIB" == "debian-32" ]]; then
+        #     ADDITIONAL_PIP_OPTIONS="$ADDITIONAL_PIP_OPTIONS --check-build-dependencies"
+        # fi


I don't understand the TODO comment: it's a temporary hack but actually it's commented out. Do you mean that we we would like to pass --check-build-dependencies but it's not possible because of a limitation of meson.build?

Good point I forgot this, let me fix this.

Basically what happens following the Debian 32 bit Docker image update:

pip is recent enough to have --check-build-dependencies

system numpy is too old (1.24) to satisfy the pyproject.toml build dependencies (numpy>=1.25) so using pip install --check-build-dependencies will result in an error

Previously (before the Debian 32 bit Docker image update), pip was too old to have --check-build-dependencies flag ...

The temporary hack was to never add --check-build-dependencies as it will be removed in #28721 anyway. I can do a bit less hacky I think.

I ended up removing this since there is no need to use --check-build-dependencies in the CI. This was mostly intended to have user friendly error when your build dependencies are not recent enough but it has limitation and #28721 will remove --check-build-dependencies everywhere soonish.

…nto remove-setup-py

dotting the i's and crossing the t's Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

…as this will go away with scikit-learn#28721

jeremiedbb

LGTM

lesteve · 2024-07-11T06:34:34Z

Merging my own PR with 3 approvals. Bye bye setuptools!

lesteve added 2 commits July 3, 2024 14:02

BLD Remove support for setuptools build

d29edd2

[azure parallel] [cd build gh]

4bd20b6

lesteve changed the title ~~Remove setup py~~ BLD Remove support for setuptools build Jul 3, 2024

[cd build gh] [azure parallel] fix

b25b02b

jeremiedbb reviewed Jul 3, 2024

View reviewed changes

lesteve added 3 commits July 3, 2024 15:38

Add meson-python to min-deps build

14efddb

[cd build gh] [azure parallel] tweaks

846e4ae

[cd build gh] [azure parallel] Add changelog

18ee47c

lesteve added 10 commits July 3, 2024 16:55

[azure parallel] fix

006251c

[azure parallel] fix

c125cd7

[azure parallel] fix

73a0bdf

[azure parallel] debug

dc10b7b

[azure parallel] increase tol

699bc42

Merge branch 'main' into remove-setup-py

ef65239

Attempt more recent Debian for 32bit build

3daa371

Add newline to Makefile

271b4c5

[azure parallel] Still need the tol increase with more recent Debian

9c203b3

[azure parallel] [cd build gh]

25743a5

thomasjpfan approved these changes Jul 9, 2024

View reviewed changes

ogrisel approved these changes Jul 9, 2024

View reviewed changes

lesteve and others added 4 commits July 9, 2024 17:20

Merge branch 'main' of https://github.com/scikit-learn/scikit-learn i…

b2a873c

…nto remove-setup-py

[azure parallel] Update lock-file

7eb3d39

[azure parallel] Less hacky

6b3fd99

Apply suggestions from code review

7275616

dotting the i's and crossing the t's Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

lesteve force-pushed the remove-setup-py branch from 8e3c0f3 to 7275616 Compare July 9, 2024 15:32

[azure parallel] [cd build gh] Remove the --check-build-dependencies …

c147b33

…as this will go away with scikit-learn#28721

jeremiedbb reviewed Jul 10, 2024

View reviewed changes

Comment thread build_tools/azure/install.sh

jeremiedbb approved these changes Jul 10, 2024

View reviewed changes

lesteve merged commit 7eb7eff into scikit-learn:main Jul 11, 2024

lesteve deleted the remove-setup-py branch July 11, 2024 06:34

lesteve mentioned this pull request Aug 3, 2024

CI Use sys.monitoring with coverage to speed-up Python >= 3.12 builds #29473

Merged

glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Sep 9, 2024

TST fix tolerance as in scikit-learn#29400

222cc64

glemaitre added a commit that referenced this pull request Sep 11, 2024

TST fix tolerance as in #29400

4d838dc

Uh oh!

Conversation

lesteve commented Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✔️ Linting Passed

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!

lesteve Jul 3, 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!

lesteve commented Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lesteve Jul 9, 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!

Uh oh!

jeremiedbb left a comment

Choose a reason for hiding this comment

Uh oh!

lesteve commented Jul 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lesteve commented Jul 3, 2024 •

edited

Loading

github-actions Bot commented Jul 3, 2024 •

edited

Loading

lesteve Jul 3, 2024 •

edited

Loading

lesteve commented Jul 3, 2024 •

edited

Loading

lesteve Jul 9, 2024 •

edited

Loading