✔️ Linting Passed

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

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

Weird failure in Ubuntu Atlas CI build doesn't seem related ? From failing build log. Merging main build log seems all fine 🤔 🤷 ...

_____ test_ridge_regression_check_arguments_validity[sag-array-None-False] _____
[gw0] linux -- Python 3.12.3 /home/vsts/work/1/s/testvenv/bin/python

return_intercept = False, sample_weight = None
        equal_nan  = True
        err_msg    = ''
        rtol       = 0
        verbose    = True
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

args = (<function assert_allclose.<locals>.compare at 0x7fc514a2ede0>, array([0.99996399, 2.00006083, 0.10015876]), array([1. , 2. , 0.1]))
kwds = {'equal_nan': True, 'err_msg': '', 'header': 'Not equal to tolerance rtol=0, atol=0.0001', 'verbose': True}

    @wraps(func)
    def inner(*args, **kwds):
        with self._recreate_cm():
>           return func(*args, **kwds)
E           AssertionError: 
E           Not equal to tolerance rtol=0, atol=0.0001
E           
E           Mismatched elements: 1 / 3 (33.3%)
E           Max absolute difference: 0.00015876
E           Max relative difference: 0.00158762
E            x: array([0.999964, 2.000061, 0.100159])
E            y: array([1. , 2. , 0.1])

args       = (<function assert_allclose.<locals>.compare at 0x7fc514a2ede0>, array([0.99996399, 2.00006083, 0.10015876]), array([1. , 2. , 0.1]))
func       = <function assert_array_compare at 0x7fc5255cac00>
kwds       = {'equal_nan': True, 'err_msg': '', 'header': 'Not equal to tolerance rtol=0, atol=0.0001', 'verbose': True}
self       = <contextlib._GeneratorContextManager object at 0x7fc5255a7320>

/usr/lib/python3.12/contextlib.py:81: AssertionError

cc @rgommers in case you have heard of similar cases of meson generated files path length limitations on Windows. Also do you think it is worth opening a meson issue to ask if they would consider creating the generate .c or .cpp files in a less nested folder (maybe there is a reason behind it, not sure ...)?

I haven't run into this before. I agree the generated paths look overly long, so perhaps that can be improved. @eli-schwartz may be able to confirm. The subfolder name repetition after path/to/target_filename.p/ looks unnecessary.

Previously: mesonbuild/meson#10788

The subfolder name repetition after path/to/target_filename.p/ looks unnecessary.

This may possibly be motivated by a desire to identify and disambiguate between the possibility of having a single target which compiles multiple source files:

• foo/bar/util.pyx
• foo/baz/util.pyx

They can't both be generated outright as

build/mytarget.cpython-313-x86_64-linux-gnu.so.p/util.pyx.cpp

And the "simplest" answer for how to distinguish them is "just add the original path in there, and people can also investigate the build by simply matching the source path with the generated path".

At least in this case you don't have to deal with both path_max and filename_max...

@lesteve as suggested in the comment.
And despite the fact that I do not understand much about compilers, I could validate the situation that I've faced with MAX_PATH in Windows, and your solution solved, at least what I faced in the past.

I created a "long path" C:\Users\vitor.pohlenz\Documents\Python\long_path_to_test\scikit-learn and it breaks in the current version of v1.6.1 as #31149 comment.
But it works completely fine with your modifications in this PR (at least on my "machine" and "long path").

Thanks @vitorpohlenz for taking the time to test this PR 🙏! I was rather confident the work-around in this PR would make it less likely to hit long path limitation on Windows, but it's definitely useful to have an external confirmation.

This may possibly be motivated by a desire to identify and disambiguate between the possibility of having a single target which compiles multiple source files:

* foo/bar/util.pyx

* foo/baz/util.pyx

@eli-schwartz thanks for your inputs, super useful! I indeed thought about something like this but my guess is that using two .pyx in one extension module is actually is not possible in Cython (I am certainly not a Cython expert unfortunately so this would definitely need double-checking). Even if this was a Cython feature, I would expect that very few projects use it, since scipy and scikit-learn don't use it.

@eli-schwartz I had a quick and dirty attempt that seems to agree with my initial intuition that it isn't possible to have two .pyx in a single extension module, see below.

This SO question seems to go towards the same direction although I have to admit I did not read everything in details 😅.

Here is my quick and dirty attempt, I added the following to sklearn/cluster/meson.build and everything builds fine.

py.extension_module(
  'combined',
  ['test1.pyx', 'test2.pyx'],
  cython_args: cython_args,
  subdir: 'sklearn/cluster',
  install: true,
)


py.extension_module(
  'test1',
  ['test1.pyx'],
  cython_args: cython_args,
  subdir: 'sklearn/cluster',
  install: true,
)


py.extension_module(
  'test2',
  ['test2.pyx'],
  cython_args: cython_args,
  subdir: 'sklearn/cluster',
  install: true,
)

Here is what I get when I try to import the built modules:

• python -c 'from sklearn.cluster import test1, test2' works fine
• python -c 'from sklearn.cluster import combined' errors:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
    from sklearn.cluster import combined
ImportError: dynamic module does not define module export function (PyInit_combined)

The content of the test{1,2}.pyx for explicitness:

# test1.pyx
def test1(): pass

# test2.pyx
def test2(): pass

Indeed, it's not possible to combine two .pyx files AFAIK. It wouldn't work without special handling, since you must have exactly one PyInit_* function per extension module.

@eli-schwartz thanks for your inputs, super useful! I indeed thought about something like this but my guess is that using two .pyx in one extension module is actually is not possible in Cython (I am certainly not a Cython expert unfortunately so this would definitely need double-checking). Even if this was a Cython feature, I would expect that very few projects use it, since scipy and scikit-learn don't use it.

Given that pxd exists I suppose having multiple pyx could be seen as rather redundant. The only actual potential for a distinction between the two would be the ability to parallel-compile the module unit in individual chunks.

This SO question seems to go towards the same direction although I have to admit I did not read everything in details 😅.

... which appears to be about having a single final *.so which the python interpreter can import multiple modules from, and that's indeed madness and not actually a decision that cython gets to make, but it wasn't what I was talking about.

Because it (multiple source files per installable output artifact) is, however, very much allowed for say vala, which is a language meson supports similarly to cython, in that it is transpiled to C and then the C is compiled.

Maybe it makes sense to only use the file basename, for cython specifically. That's entirely possible, and if this is changed directly in meson then you can take advantage of builtin cython support while still playing nice with the Windows tempdir etc.

Indeed, it's not possible to combine two .pyx files AFAIK. It wouldn't work without special handling, since you must have exactly one PyInit_* function per extension module.

... and e.g. vala handles this because in vala, you have to write your own main() entrypoint, thus it doesn't try to autogenerate one into every file and you don't end up with two entrypoints in one built installable artifact.

Maybe it makes sense to only use the file basename, for cython specifically.

That would indeed seem like a good option if it is feasible on the meson side.

if this is changed directly in meson then you can take advantage of builtin cython support while still playing nice with the Windows tempdir etc.

I will be AFK for a few weeks, so it's definitely not going to happen short-term, but it did not seem like the automatic shortening of path from mesonbuild/meson#10788 actually was kicking in in the issues we saw on Windows in a scikit-learn context. Maybe the heuristics of for "filepaths with >5 components" was not met? Full disclosure: I did not read mesonbuild/meson#10788 in enough details, so I'll try to look at it in more details when I am back.