Skip to content

Build with pinned llvm-openmp #265

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 23, 2024
Merged

Build with pinned llvm-openmp #265

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6486fd5
Build with pinned llvm-openmp
xhochy May 22, 2024
b124302
Add comment about duplication
xhochy May 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion recipe/meta.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ source:
sha256: cd2aac2b566c7e740d34aabb4737864c74ba33c29aad7101fbf1bab2931c02dc

build:
number: 0
number: 1
skip: true # [py<39]

requirements:
Expand Down Expand Up @@ -40,6 +40,9 @@ requirements:
- scipy >=1.8 # [aarch64]
- joblib >=1.2.0
- threadpoolctl >=3.1.0
# See https://github.com/conda-forge/openmp-feedstock/issues/126
- llvm-openmp # [osx]
- libgomp # [linux]
Comment on lines +44 to +45
Copy link
Member

Choose a reason for hiding this comment

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

The same specifications are present in the build section.

Should it be removed for the current on that you propose?

Copy link
Member

@jjerphan jjerphan May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@xhochy: why mustn't the specification line 28 and line 29 be removed?

Edit: they must be present in this section, see the reasons in the documentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes! In the perfect world, they would suffice.

Copy link
Member

@jakirkham jakirkham May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Could we just add the desired pin in build?

Should add I'm confused as to why the requirement in build is not already aligned with the compiler, which it is installed next to

Edit: Maybe an example of the issue encountered would help. Is there one somewhere we can look at?

Copy link
Member Author

Choose a reason for hiding this comment

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

I outlined the issue in conda-forge/openmp-feedstock#126 We actually install the desired pin in build but due to conda-builds internal behaviour, the run_exports is still taken from host. Thus, I don't see a solution where we don't add anything to host.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok. Thank you Uwe! Very helpful 🙂

Wrote up a reply there on what we could consider next: conda-forge/openmp-feedstock#126 (comment)

Agree this is the best we can do for now

Comment on lines +43 to +45
Copy link
Member

@jakirkham jakirkham May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do we need to do something like this (though maybe with tweaks) to better explicitly align versions between build & host?

Suggested change
# See https://github.com/conda-forge/openmp-feedstock/issues/126
- llvm-openmp # [osx]
- libgomp # [linux]
# See https://github.com/conda-forge/openmp-feedstock/issues/126
- llvm-openmp {{ llvm_openmp }} # [osx]
- libgomp {{ libgomp }} # [linux]

Edit: This may need to be done with the compiler versions themselves

Suggested change
# See https://github.com/conda-forge/openmp-feedstock/issues/126
- llvm-openmp # [osx]
- libgomp # [linux]
# See https://github.com/conda-forge/openmp-feedstock/issues/126
- llvm-openmp {{ c_compiler_version }} # [osx]
- libgomp {{ c_compiler_version }} # [linux]

Copy link
Member Author

Choose a reason for hiding this comment

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

No, they are already pinned in .ci_support:

scikit-learn-feedstock/.ci_support/osx_64_numpy2.0python3.11.____cpython.yaml

Lines 21 to 22 in 8bb0be3

llvm_openmp:
- '16'

Copy link
Member

Choose a reason for hiding this comment

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

Is that sufficiently pinned for our needs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes!

Copy link
Member

Choose a reason for hiding this comment

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

Sorry. Asking because this is only the major version and wasn't sure if that is enough for the use case that you asked about. If it is, great!

run:
- python
- scipy
Expand Down