BLD Reduces size of wheels by stripping symbols #25123

thomasjpfan · 2022-12-06T20:15:52Z

Reference Issues/PRs

Follow up to #25063

What does this implement/fix? Explain your changes.

This PR sets -g0 to strip symbols and reduce the file size of the linux wheels.

thomasjpfan · 2022-12-06T21:29:18Z

@jeremiedbb @ogrisel This resolves the large wheel size problem on Linux. I think it would be nice to have for 1.2

thomasjpfan · 2022-12-06T21:32:57Z

From this PR, the total size of the artifacts zip is 142 MB. The latest wheel build in main was 231 MB.

When I unzip the artifacts and look at the wheels, I see that the manylinux wheels are around the same size as Windows and macOS. On main, the Linux wheels were 36.6 MB. With this PR, the Linux wheels are now 9.8 MB.

jeremiedbb · 2022-12-06T22:34:33Z

I think it's fine to disable all debug informations for the wheels but I'd like to have an easy way to re-enable them when building in my dev env. Maybe we can have an environnement variable ?

thomasjpfan · 2022-12-06T23:10:00Z

I updated the PR to support --debug which is used to build with symbols:

python setup.py build_ext -i --debug

Are you okay with this as in a dev environment?

Maybe we can have an environnement variable ?

If the above is not agreeable, then I'm okay with adding a environment variable, such as SKLEARN_ENABLE_DEBUG_SYMBOLS.

ogrisel · 2022-12-06T23:26:40Z

Let's use the SKLEARN_BUILD_ prefix for env variables that only impact at build time.

ogrisel · 2022-12-06T23:30:08Z

About the change itself I wonder if it won't impact profilers with native code profiling such as py-spy with the --native flag on linux.

thomasjpfan · 2022-12-07T00:33:55Z

Coming from py-spy's README, stripping symbols will not give the best output. If a developer is using py-spy, it is best to build scikit-learn with symbols on.

Overall, I think it is still better to strip the symbols for the wheels on PyPI because:

It is consistent with the macOS and Windows wheels
NumPy and SciPy also strip the symbols
Better for users with lower internet bandwidth

I updated this PR, introducing a SKLEARN_BUILD_ENABLE_DEBUG_SYMBOLS to control the striping of the symbols.

adrinjalali

I'm happy with the change, but we should make sure the env variables we have around the codebase are discoverable.

adrinjalali · 2022-12-07T08:46:22Z

doc/computing/parallelism.rst

+`SKLEARN_BUILD_ENABLE_DEBUG_SYMBOLS`
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+When this environment variable is set to a non zero value, the debug symbols
+will be included in the compiled C extensions. Only debug symbols for POSIX
+systems is configured.


this doesn't really belong to parallelism.rst though.

Although this file is named parallelism.rst, the title is "Parallelism, resource management, and configuration". I'm guessing all the environment variables listed here fall under "configuration", which this PR adds onto.

As for discoverability, searching for "configuration" places the parallelism.rst page as second. I'm open to figuring out a better location for all the configuration options, but I do not think it is a blocker for this PR.

adrinjalali · 2022-12-07T12:14:03Z

This would be a significant improvement on the inference endpoints I have which create a new mamba environment every time, so it makes me happy :D

EDIT: oh no, this doesn't affect conda-forge packages lol. Silly me.

jeremiedbb

I liked the --debug option. @ogrisel was your remark about the env var just about the name of such a var or that you prefer the env var over this cmd line option ?

jeremiedbb · 2022-12-07T12:37:57Z

setup.py

+        if build_with_debug_symbols:
+            default_extra_compile_args.append("-g")


is it not already set by default ?

On macOS+clang and Windows+MSCV, the debug symbols are stripped by default, which resulted in the ~9MB wheels on PyPI. With the new environment variable, I can enable debug symbols without changing the source on my local macOS machine.

ah yes, right

jjerphan

LGTM as a first iteration to integrate as part of 1.2.

Further improvements can be:

restructuring information in parallelism.rst which is not about parallelism anymore
using binary stripper, like strip(1) to further reduce the size of wheel-shipped shared objects

Edit: I let @jeremiedbb merge if this LGTH.

jeremiedbb · 2022-12-07T16:55:00Z

I let @jeremiedbb merge if this LGTH.

I just wonder why we ruled out the --debug flag that quickly.

adrinjalali · 2022-12-07T16:56:51Z

I just wonder why we ruled out the --debug flag that quickly.

I second that. I have a slight preference for it since it sounds more standard, but no strong opinion.

thomasjpfan · 2022-12-07T17:13:04Z

I just wonder why we ruled out the --debug flag that quickly.

With --debug, it's harder to build scikit-learn with debug symbols through pyproject.toml. I can imagine a use case of someone using py-spy and need debug symbols with --native. With the environment variable, they can set the variable and run pip install git+https://github.com/scikit-learn/scikit-learn and it "just works".

What makes build_ext --debug harder is that it requires more knowledge about setuptools + setup.py and how the build process works to get symbols installed.

jeremiedbb · 2022-12-07T17:18:54Z

Thanks for the clarification. Let's merge then :)

thomasjpfan added 2 commits December 6, 2022 15:13

BLD Strip symbols

4f62f3a

CI [cd build]

173af77

thomasjpfan added this to the 1.2 milestone Dec 6, 2022

thomasjpfan mentioned this pull request Dec 6, 2022

Release 1.2.0 #25121

Merged

11 tasks

thomasjpfan added 2 commits December 6, 2022 18:04

ENH Allow --debug to enable symbols [cd build gh]

4aaa016

DOC Minor rewording [cd build gh]

66b5507

thomasjpfan added 2 commits December 6, 2022 18:39

DOC Add more details [cd build gh]

0b9a92a

ENH Adds SKLEARN_BUILD_ENABLE_DEBUG_SYMBOLS [cd build gh]

49b9465

adrinjalali reviewed Dec 7, 2022

View reviewed changes

CLN Move closer to other flags [cd build gh]

ff2dc70

adrinjalali approved these changes Dec 7, 2022

View reviewed changes

jeremiedbb reviewed Dec 7, 2022

View reviewed changes

jjerphan approved these changes Dec 7, 2022

View reviewed changes

jeremiedbb added the To backport PR merged in master that need a backport to a release branch defined based on the milestone. label Dec 7, 2022

jeremiedbb merged commit 9527920 into scikit-learn:main Dec 7, 2022

jeremiedbb pushed a commit to jeremiedbb/scikit-learn that referenced this pull request Dec 7, 2022

BLD Reduces size of wheels by stripping symbols (scikit-learn#25123)

cc05e7e

Vincent-Maladiere pushed a commit to Vincent-Maladiere/scikit-learn that referenced this pull request Dec 14, 2022

BLD Reduces size of wheels by stripping symbols (scikit-learn#25123)

1abd5df

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BLD Reduces size of wheels by stripping symbols #25123

BLD Reduces size of wheels by stripping symbols #25123

thomasjpfan commented Dec 6, 2022

thomasjpfan commented Dec 6, 2022 •

edited

thomasjpfan commented Dec 6, 2022 •

edited

jeremiedbb commented Dec 6, 2022

thomasjpfan commented Dec 6, 2022

ogrisel commented Dec 6, 2022

ogrisel commented Dec 6, 2022

thomasjpfan commented Dec 7, 2022

adrinjalali left a comment

adrinjalali Dec 7, 2022

thomasjpfan Dec 7, 2022 •

edited

thomasjpfan Dec 7, 2022 •

edited

adrinjalali commented Dec 7, 2022 •

edited

jeremiedbb left a comment

jeremiedbb Dec 7, 2022

thomasjpfan Dec 7, 2022

jeremiedbb Dec 7, 2022

jjerphan left a comment •

edited

jeremiedbb commented Dec 7, 2022 •

edited

adrinjalali commented Dec 7, 2022

thomasjpfan commented Dec 7, 2022 •

edited

jeremiedbb commented Dec 7, 2022

		if build_with_debug_symbols:
		default_extra_compile_args.append("-g")

BLD Reduces size of wheels by stripping symbols #25123

BLD Reduces size of wheels by stripping symbols #25123

Conversation

thomasjpfan commented Dec 6, 2022

Reference Issues/PRs

What does this implement/fix? Explain your changes.

thomasjpfan commented Dec 6, 2022 • edited

thomasjpfan commented Dec 6, 2022 • edited

jeremiedbb commented Dec 6, 2022

thomasjpfan commented Dec 6, 2022

ogrisel commented Dec 6, 2022

ogrisel commented Dec 6, 2022

thomasjpfan commented Dec 7, 2022

adrinjalali left a comment

Choose a reason for hiding this comment

adrinjalali Dec 7, 2022

Choose a reason for hiding this comment

thomasjpfan Dec 7, 2022 • edited

Choose a reason for hiding this comment

thomasjpfan Dec 7, 2022 • edited

Choose a reason for hiding this comment

adrinjalali commented Dec 7, 2022 • edited

jeremiedbb left a comment

Choose a reason for hiding this comment

jeremiedbb Dec 7, 2022

Choose a reason for hiding this comment

thomasjpfan Dec 7, 2022

Choose a reason for hiding this comment

jeremiedbb Dec 7, 2022

Choose a reason for hiding this comment

jjerphan left a comment • edited

Choose a reason for hiding this comment

jeremiedbb commented Dec 7, 2022 • edited

adrinjalali commented Dec 7, 2022

thomasjpfan commented Dec 7, 2022 • edited

jeremiedbb commented Dec 7, 2022

thomasjpfan commented Dec 6, 2022 •

edited

thomasjpfan commented Dec 6, 2022 •

edited

thomasjpfan Dec 7, 2022 •

edited

thomasjpfan Dec 7, 2022 •

edited

adrinjalali commented Dec 7, 2022 •

edited

jjerphan left a comment •

edited

jeremiedbb commented Dec 7, 2022 •

edited

thomasjpfan commented Dec 7, 2022 •

edited