GCC 14: Remove `inline` for `rdist_csr` and `rdist` #28541

hswong3i · 2024-02-27T04:31:01Z

Reference Issues/PRs

Fedora bug: https://bugzilla.redhat.com/show_bug.cgi?id=2261602

What does this implement/fix? Explain your changes.

This problem was mentioned in a bug open to keep track of the effort to remove all compilation warnings #24875 (search "Incompatible pointers types casts")

With gcc < 14 the code compiles with incompatible-pointer warning. But with gcc 14 that warning is an error.

A possible workaround is to remove the inline in the definition of the rdist_csr and rdist members.

With that change, cython generates correct code (I don't know why). But removing the inline can impact the performance.

Any other comments?

This is related with cython/cython#2747 This problem was mentioned in a bug open to keep track of the effort to remove all compilation warnings scikit-learn#24875 (search "Incompatible pointers types casts") With gcc < 14 the code compiles with incompatible-pointer warning. But with gcc 14 that warning is an error. Fedora bug: https://bugzilla.redhat.com/show_bug.cgi?id=2261602 A posible workaround is to remove the inline in the definition of the rdist_csr and rdistmembers. With that change, cython generates correct code (I don't know why). But removing the inline can impact the performance. Fixes scikit-learn#28530 Signed-off-by: Wong Hoi Sing Edison <hswong3i@pantarei-design.com>

github-actions · 2024-02-27T04:32:19Z

✔️ Linting Passed

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

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

hswong3i · 2024-02-27T04:36:26Z

Testing with podman + Fedora Rawhide:

sudo -E podman run -ti --rm fedora:rawhide
yum update -y
yum install -y git gcc gcc-c++ python3-devel python3-pip python3-setuptools python3-wheel python3-Cython python3-numpy python3-scipy python3-joblib python3-threadpoolctl
git clone https://github.com/scikit-learn/scikit-learn.git
cd scikit-learn
git checkout main
curl -skL https://github.com/scikit-learn/scikit-learn/pull/28541.patch | patch -p1
python3 setup.py build

git clean -xdf git submodule sync git submodule update --init tar zcvf ../python-scikit-learn_1.3.2.orig.tar.gz --exclude=.git . debuild -uc -us cp python-scikit-learn.spec ../python-scikit-learn_1.3.2-1.spec cp ../python*-scikit-learn*1.3.2*.{gz,xz,spec,dsc} /osc/home\:alvistack/scikit-learn-scikit-learn-1.3.2/ rm -rf ../python*-scikit-learn*1.3.2*.* ../python*-sklearn*1.3.2*.* See scikit-learn#28541 Signed-off-by: Wong Hoi Sing Edison <hswong3i@pantarei-design.com>

git clean -xdf git submodule sync git submodule update --init tar zcvf ../python-scikit-learn_1.4.0.orig.tar.gz --exclude=.git . debuild -uc -us cp python-scikit-learn.spec ../python-scikit-learn_1.4.0-1.spec cp ../python*-scikit-learn*1.4.0*.{gz,xz,spec,dsc} /osc/home\:alvistack/scikit-learn-scikit-learn-1.4.0/ rm -rf ../python*-scikit-learn*1.4.0*.* ../python*-sklearn*1.4.0*.* See scikit-learn#28541 Signed-off-by: Wong Hoi Sing Edison <hswong3i@pantarei-design.com>

git clean -xdf git submodule sync git submodule update --init tar zcvf ../python-scikit-learn_1.4.1.post1.orig.tar.gz --exclude=.git . debuild -uc -us cp python-scikit-learn.spec ../python-scikit-learn_1.4.1.post1-1.spec cp ../python*-scikit-learn*1.4.1.post1*.{gz,xz,spec,dsc} /osc/home\:alvistack/scikit-learn-scikit-learn-1.4.1.post1/ rm -rf ../python*-scikit-learn*1.4.1.post1*.* ../python*-sklearn*1.4.1.post1*.* See scikit-learn#28541 Signed-off-by: Wong Hoi Sing Edison <hswong3i@pantarei-design.com>

betatim · 2024-02-27T08:16:04Z

Thanks for sending this patch! Let's see if we can get people who are experts in this code interested.

@Micky774 and @jjerphan could you make some time to think about this? In particular performance related things. For example is there a benchmark that we can run to measure the difference?

jjerphan · 2024-02-27T08:47:57Z

I would run benchmarks from scikit-learn/pairwise-distances-reductions-asv-suite potentially updating the parameter list to only test for a single metric which is not "euclidean" because EuclideanDistance is not used in this case (e.g. "minkowski" would test MinkowskiDistance).

I do not have time to have a look at that for now. :/

glemaitre · 2024-02-27T08:49:00Z

But removing the inline can impact the performance.

Indeed, we should pay some additional overhead. Since this is impacting the rdist, we should run a benchmark with a radius neighbors to see the impact of this regression. It is clear that the best solution is to have a fix in Cython but currently, we don't allow some stack to package scikit-learn and I don't see any other workaround.

jjerphan · 2024-02-27T08:52:09Z

Another solution would be to have the presence of inline be compiler-dependent.

I think we should only consider this second solution if the present one causes regressions.

betatim · 2024-02-27T09:05:51Z

I'm not sure about all compilers (and all versions of them) but some compilers will consider any function, with or without inline for inlining. For example in GCC there is -finline-functions which is part of -O2and -O3. I don't know if the heuristic it uses is different from the one used for functions marked as inline (which is just a hint to the compiler, it doesn't mean the function will actually be inlined)?

So I think it is possible that we find that removing the inline definition does not reduce performance. And if that is the case I think we should remove them instead of building a more complicated system/workaround.

jjerphan · 2024-02-28T09:58:12Z

If cython/cython#6039 is merged and a new version of Cython released, then we may skip this PR.

betatim · 2024-02-28T10:38:34Z

If cython/cython#6039 is merged and a new version of Cython released, then we may skip this PR.

From a quick look at the PR and the discussion that lead to it: it seems like this just suppresses the warning? Is that right? I tried to find out more about the warning and why gcc warns you about this. But didn't find out much :( My question is: is this warning a false positive for scikit-learn's code or is it a true positive? If the latter, shouldn't we fix the source of the warning instead of turning off the warning? The "fix instead of suppress" attitude comes from the fact that usually gcc has a good reason to warn about something, even if it is annoying ;)

jjerphan · 2024-02-28T10:45:14Z

If the latter, shouldn't we fix the source of the warning instead of turning off the warning?

We can fix the problem in scikit-learn with this PR, but to me the issue is Cython's.

I do not know sufficiently about Cython to be able to tell if the original problem is addressable or not to properly choose a solution.

What do you think?

jjerphan · 2024-02-28T14:23:29Z

I cannot run the tests suite because Cython fails to compile scikit-learn for other reasons.

Here are the changes which I think need to be made to scikit-learn/pairwise-distances-reductions-asv-suite to test for regressions:

diff --git i/asv.conf.json w/asv.conf.json
index e647afb..1469459 100644
--- i/asv.conf.json
+++ w/asv.conf.json
@@ -11,7 +11,7 @@
 
     // The URL or local path of the source code repository for the
     // project being benchmarked
-    "repo": "https://github.com/scikit-learn/scikit-learn.git",
+    "repo": "https://github.com/alvistack/scikit-learn-scikit-learn.git",
 
     // The Python project's subdirectory in your repo.  If missing or
     // the empty string, the project is assumed to be located at the root
diff --git i/benchmarks/pairwise_distances_reductions.py w/benchmarks/pairwise_distances_reductions.py
index 62b952c..7b63433 100644
--- i/benchmarks/pairwise_distances_reductions.py
+++ w/benchmarks/pairwise_distances_reductions.py
@@ -23,12 +23,12 @@ class PairwiseDistancesReductionsBenchmark(Benchmark):
         "X_test",
     ]
     params = [
-        [1000, 10_000, int(1e7)],
-        [1000, 10_000, 100_000],
-        [100],
-        ["euclidean", "manhattan"],
-        ["auto", "parallel_on_X", "parallel_on_Y"],
-        [np.float32, np.float64],
+        [1000, 10_000],
+        [10_000, 100_000],
+        [10],
+        ["manhattan"],
+        ["auto"],
+        [np.float32],
         ["dense", "csr"],
         ["dense", "csr"],
     ]

and then one can run:

asv continuous main main-gcc14

lorentzenchr · 2024-02-28T20:07:15Z

@da-woods Could you help out here? (In the past you did quite many times!)

da-woods · 2024-02-28T20:35:54Z

The warning's a "false" positive (i.e. it isn't hiding any real bugs in this case, but the C compiler is right to warn). So I wouldn't be worried about just suppressing it.

but to me the issue is Cython's.

Yes - this is right.

I think the plan is to get the warning at least suppressed in the next Cython release (which I think will probably be today or tomorrow). If I were you I'd just wait for that unless you're in a huge rush.

It is something that Cython should follow up and fix properly in the future though.

lorentzenchr · 2024-02-28T20:39:42Z

@da-woods Thank you so much.

ogrisel · 2024-03-20T17:11:29Z

I believe we don't need this PR anymore with Cython 3.0.9 as said here:

#28530 (comment)

So let me close. We can reopen if I am wrong.

github-actions bot added module:metrics cython labels Feb 27, 2024

hswong3i mentioned this pull request Feb 27, 2024

BUILD gcc14 cannot compile scikit-learn #28530

Closed

ogrisel closed this Mar 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GCC 14: Remove `inline` for `rdist_csr` and `rdist` #28541

GCC 14: Remove `inline` for `rdist_csr` and `rdist` #28541

hswong3i commented Feb 27, 2024 •

edited by jjerphan

Loading

github-actions bot commented Feb 27, 2024

hswong3i commented Feb 27, 2024

betatim commented Feb 27, 2024

jjerphan commented Feb 27, 2024

glemaitre commented Feb 27, 2024

jjerphan commented Feb 27, 2024 •

edited

Loading

betatim commented Feb 27, 2024 •

edited

Loading

jjerphan commented Feb 28, 2024

betatim commented Feb 28, 2024

jjerphan commented Feb 28, 2024 •

edited

Loading

jjerphan commented Feb 28, 2024 •

edited

Loading

lorentzenchr commented Feb 28, 2024

da-woods commented Feb 28, 2024

lorentzenchr commented Feb 28, 2024

ogrisel commented Mar 20, 2024

GCC 14: Remove inline for rdist_csr and rdist #28541

GCC 14: Remove inline for rdist_csr and rdist #28541

Conversation

hswong3i commented Feb 27, 2024 • edited by jjerphan Loading

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

github-actions bot commented Feb 27, 2024

✔️ Linting Passed

hswong3i commented Feb 27, 2024

betatim commented Feb 27, 2024

jjerphan commented Feb 27, 2024

glemaitre commented Feb 27, 2024

jjerphan commented Feb 27, 2024 • edited Loading

betatim commented Feb 27, 2024 • edited Loading

jjerphan commented Feb 28, 2024

betatim commented Feb 28, 2024

jjerphan commented Feb 28, 2024 • edited Loading

jjerphan commented Feb 28, 2024 • edited Loading

lorentzenchr commented Feb 28, 2024

da-woods commented Feb 28, 2024

lorentzenchr commented Feb 28, 2024

ogrisel commented Mar 20, 2024

GCC 14: Remove `inline` for `rdist_csr` and `rdist` #28541

GCC 14: Remove `inline` for `rdist_csr` and `rdist` #28541

hswong3i commented Feb 27, 2024 •

edited by jjerphan

Loading

jjerphan commented Feb 27, 2024 •

edited

Loading

betatim commented Feb 27, 2024 •

edited

Loading

jjerphan commented Feb 28, 2024 •

edited

Loading

jjerphan commented Feb 28, 2024 •

edited

Loading