Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require libgomp on Linux #220

Merged
merged 3 commits into from
May 25, 2023

Conversation

jakirkham
Copy link
Member

@jakirkham jakirkham commented May 24, 2023

Looks like OpenMP is used for the builds on macOS, but the dependency is not included on Linux. This adds libgomp on Linux to provide OpenMP support on that platform.


Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

To satisfy the OpenMP dependency on Linux too, add `libgomp` to the
requirements.
Rebuild packages with `libgomp` dependency on Linux.
@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@jjerphan
Copy link
Member

Hi @jakirkham, thank you for contributing.

Have you observed any problems at runtime with the current build (e.g. no thread parallelism?).

@jakirkham
Copy link
Member Author

Thanks Julien! 🙏

A colleague was seeing a rather strange error message in relation to scinkit-learn usage ( dask-contrib/dask-sql#1144 (comment) ), which when digging more deeply can be caused by GNU OpenMP either being missing or not found correctly ( for example: opencv/opencv#14884 (comment) )

As LLVM OpenMP is included here for macOS and the usual recommendation suggests requiring GNU OpenMP on Linux too, thought we could give this change a try

It's possible the issue encountered has other causes, but this would at least rule out one (while improving the package here as well)

Would be curious to hear your thoughts on all of this? 🙂

Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for providing context.

scikit-learn's builds for Linux distributed on conda-forge has been depending on libgomp implicitly since the support of OpenMP but I think that it is appropriate specifying it explicitly for portability (since it allows switching to llvm-openmp if needed).

I am waiting for another maintainer's approval before merging.

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change makes sense to me. LGTM!

@thomasjpfan thomasjpfan merged commit c234a18 into conda-forge:main May 25, 2023
@jakirkham jakirkham deleted the req_openmp_linux branch May 25, 2023 16:38
@jakirkham
Copy link
Member Author

Thanks Julien & Thomas! 🙏

@jjerphan
Copy link
Member

Thank you for resolving problems across packages in the ecosystem, John.

I hope to see you at the Scientific Python Developer Summit. 🌳

@jakirkham
Copy link
Member Author

Ofc!

Hoping to come over there today 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants