BLD Add Meson OpenMP checks #29762

lesteve · 2024-09-01T14:43:49Z

This avoids to mess up Meson OpenMP to try to alleviate @adrinjalali's concern and avoid missing OpenMP dependencies as in #29694.

Summary:

a git grep command is used to figure out which Cython files are using OpenMP
meson introspect produces a json that shows the ninja.build information in particular the compiler flags, so we can figure out which Cython module use OpenMP flags.

The script makes sure that 1. and 2. match.

The script also found some cases where openmp_dep was added to an extension module dependency in meson.build and the Cython file was not using any OpenMP features (based on git grep regex) so this PR fixes this.

github-actions · 2024-09-01T14:45:01Z

✔️ Linting Passed

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

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

lesteve · 2024-09-02T10:00:40Z

@adrinjalali do you want to have a quick look and see whether that seems to go in the right direction to make sure we correctly define OpenMP dependencies. I need to clean up a few things but I think this is pretty much working right now.

build_tools/meson-openmp-linter.py

thomasjpfan · 2024-09-02T12:26:34Z

build_tools/meson-openmp-linter.py

+
+
+def get_canonical_name_meson(target, build_path):
+    # TODO taking [0] I don't expect to have 2 filenames here


Can we add an assert to make check this expectation?

Suggested change

# TODO taking [0] I don't expect to have 2 filenames here

# TODO taking [0] I don't expect to have 2 filenames here

assert len(target["filename"]) == 1

build_tools/meson-openmp-linter.py

thomasjpfan · 2024-09-02T12:30:56Z

azure-pipelines.yml

@@ -40,6 +40,11 @@ jobs:
    - bash: |
        ./build_tools/linting.sh
      displayName: Run linters
+    - bash: |
+        pip install ninja meson scipy


I'm surprised that SciPy is required for this check. Is this required by meson?

Since #28721 we are checking the scipy version in sklearn/meson.build (the build dependencies versions in general), so you need scipy to run meson setup.

scikit-learn/sklearn/meson.build

Lines 41 to 45 in 4aeb191

scipy_version = run_command(py,

['-c', 'import scipy; print(scipy.__version__)'], check: true).stdout().strip()

scipy_min_version = run_command(py, ['_min_dependencies.py', 'scipy'], check: true).stdout().strip()

if not scipy_version.version_compare('>=' + scipy_min_version)

error('scikit-learn requires scipy>=' + scipy_min_version + ', got ' + scipy_version + ' instead')

I agree it is a bit surprising but I am not sure this is worth complicating the meson.build.

adrinjalali · 2024-09-02T13:46:12Z

sklearn/cluster/meson.build

 }

 foreach ext_name, ext_dict : cluster_extension_metadata
  py.extension_module(
    ext_name,
    [ext_dict.get('sources'), utils_cython_tree],
-    dependencies: [np_dep, openmp_dep],
+    dependencies: [np_dep] + ext_dict.get('dependencies', []),


does this now make things more specific since we have the linter?

The linter will catch if an extension module use OpenMP flags but does not use it, where "using OpenMP" is defined by the following git grep regexp. I think it makes sense but please double-check:

git grep -lP "cython.*parallel|_openmp_helpers"

https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/cluster/_dbscan_inner.pyx and https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/cluster/_hierarchical_fast.pyx do not use OpenMP AFAICT.

adrinjalali · 2024-09-02T13:46:47Z

sklearn/metrics/_pairwise_distances_reduction/meson.build

-  dependencies: [np_dep, openmp_dep],
+  dependencies: [np_dep],


curious why you thought these are needed before then. And are they really not needed?

curious why you thought these are needed before then.

This is the most complicated meson.build we have and I would not be surprised if I cut some corners when I put it together.

And are they really not needed?

I am pretty sure they are not needed since there is nothing related to OpenMP in https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/metrics/_pairwise_distances_reduction/_datasets_pair.pyx.tp.

adrinjalali · 2024-09-02T13:47:06Z

build_tools/meson-openmp-linter.py

+from pathlib import Path
+
+
+def uses_openmp(targets):


a bit of comment as what each function does would make reading the code easier, but not a big deal.

That was part of the clean-up I had in mind indeed 😉

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

…nto meson-openmp-linter

lesteve · 2024-09-03T15:18:21Z

OK I did the clean-up I had in mind, let me know if you think more doc/clarification is needed.

…nto meson-openmp-linter

adrinjalali

Love this! 🥳

ogrisel

No time to review in details nor to test that the script works as expected myself but I spotted a quick typo:

build_tools/check-meson-openmp-dependencies.py

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

glemaitre

I could get what happens by reading the file so I'm pretty happy with the script.

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

Add Meson OpenMP checks

ad08b46

lesteve added 8 commits September 1, 2024 16:56

fix (i.e. linting should fail)

ebe2db9

Fix i.e. linting should fail for real

84bf54e

Only run OpenMP checks on Azure

99adf3f

Fix. failure still expected

d35b8bd

Fix. failure still expected

4132784

Debug

3baa3a2

Fix cases where openmp_dep was used but not needed

6daa168

remove debug

652edd7

lesteve changed the title ~~Add Meson OpenMP checks~~ BLD Add Meson OpenMP checks Sep 2, 2024

lesteve added the Build / CI label Sep 2, 2024

thomasjpfan reviewed Sep 2, 2024

View reviewed changes

adrinjalali reviewed Sep 2, 2024

View reviewed changes

lesteve and others added 6 commits September 2, 2024 15:59

Apply suggestions from code review

0580d20

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

lint

08b4c49

clean-up

2bfd584

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

c1333fa

…nto meson-openmp-linter

Remove cruft

d2e422e

Rename script

3c7e411

lesteve added 2 commits September 4, 2024 13:49

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

5a1ab7f

…nto meson-openmp-linter

[azure parallel]

aea16ad

adrinjalali approved these changes Sep 4, 2024

View reviewed changes

ogrisel reviewed Sep 4, 2024

View reviewed changes

build_tools/check-meson-openmp-dependencies.py Outdated Show resolved Hide resolved

Update build_tools/check-meson-openmp-dependencies.py

de18c4d

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

glemaitre approved these changes Sep 5, 2024

View reviewed changes

glemaitre merged commit 6032c95 into scikit-learn:main Sep 5, 2024
30 checks passed

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

BLD Add Meson OpenMP checks (scikit-learn#29762)

8713445

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

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

BLD Add Meson OpenMP checks (#29762)

0c7073d

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BLD Add Meson OpenMP checks #29762

BLD Add Meson OpenMP checks #29762

lesteve commented Sep 1, 2024 •

edited

Loading

github-actions bot commented Sep 1, 2024 •

edited

Loading

lesteve commented Sep 2, 2024

thomasjpfan Sep 2, 2024

thomasjpfan Sep 2, 2024

lesteve Sep 2, 2024

adrinjalali Sep 2, 2024

lesteve Sep 2, 2024 •

edited

Loading

adrinjalali Sep 2, 2024

lesteve Sep 2, 2024 •

edited

Loading

adrinjalali Sep 2, 2024

lesteve Sep 2, 2024

lesteve commented Sep 3, 2024

adrinjalali left a comment

ogrisel left a comment

glemaitre left a comment



		def get_canonical_name_meson(target, build_path):
		# TODO taking [0] I don't expect to have 2 filenames here

	# TODO taking [0] I don't expect to have 2 filenames here
	# TODO taking [0] I don't expect to have 2 filenames here
	assert len(target["filename"]) == 1

	scipy_version = run_command(py,
	['-c', 'import scipy; print(scipy.__version__)'], check: true).stdout().strip()
	scipy_min_version = run_command(py, ['_min_dependencies.py', 'scipy'], check: true).stdout().strip()
	if not scipy_version.version_compare('>=' + scipy_min_version)
	error('scikit-learn requires scipy>=' + scipy_min_version + ', got ' + scipy_version + ' instead')

BLD Add Meson OpenMP checks #29762

BLD Add Meson OpenMP checks #29762

Conversation

lesteve commented Sep 1, 2024 • edited Loading

github-actions bot commented Sep 1, 2024 • edited Loading

✔️ Linting Passed

lesteve commented Sep 2, 2024

thomasjpfan Sep 2, 2024

Choose a reason for hiding this comment

thomasjpfan Sep 2, 2024

Choose a reason for hiding this comment

lesteve Sep 2, 2024

Choose a reason for hiding this comment

adrinjalali Sep 2, 2024

Choose a reason for hiding this comment

lesteve Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

adrinjalali Sep 2, 2024

Choose a reason for hiding this comment

lesteve Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

adrinjalali Sep 2, 2024

Choose a reason for hiding this comment

lesteve Sep 2, 2024

Choose a reason for hiding this comment

lesteve commented Sep 3, 2024

adrinjalali left a comment

Choose a reason for hiding this comment

ogrisel left a comment

Choose a reason for hiding this comment

glemaitre left a comment

Choose a reason for hiding this comment

lesteve commented Sep 1, 2024 •

edited

Loading

github-actions bot commented Sep 1, 2024 •

edited

Loading

lesteve Sep 2, 2024 •

edited

Loading

lesteve Sep 2, 2024 •

edited

Loading