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

Update meta.yaml to rebuild with patch for SVML #73

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

xaleryb
Copy link

@xaleryb xaleryb commented Aug 4, 2023

SVMl patch was added to llvmdev by this PR conda-forge/llvmdev-feedstock#192

Idea of this PR to rebuild llvmlite with dependencies published on conda-forge channel (which includes SVML patch)

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.

SVMl patch was added to llvmdev by this PR conda-forge/llvmdev-feedstock#192

Idea of this PR to rebuild llvmlite with dependencies published on conda-forge channel (which includes SVML patch)
@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.

@xaleryb
Copy link
Author

xaleryb commented Aug 4, 2023

This issue describes our needs #72

@ekomarova
Copy link

@conda-forge-admin, please rerender

@xaleryb
Copy link
Author

xaleryb commented Aug 8, 2023

@jakirkham could you please review?

@xaleryb
Copy link
Author

xaleryb commented Aug 9, 2023

@jakirkham, @marcelotrevisani, @mbargull, @souravsingh, @xhochy please review and approve if you do not have any comments.

@xhochy
Copy link
Member

xhochy commented Aug 9, 2023

I don't understand why this needs a rebuild. Are the patches breaking ABI?

@Hardcode84
Copy link

Hardcode84 commented Aug 9, 2023

@xhochy patches technically doesn't break ABI but attempt to genrate SVML code with unpatched LLVM will lead to wrong codegen (although I'm not aware of any other projects besides llvmlite who are using SVML codegen in LLVM). lvmlite checking for patched LLVM during cmake configuration and compiles result into their binary.

UPD: Well, disregard previous comment, they are probably breaking ABI as well.

@jakirkham
Copy link
Member

Appreciate folks taking the time to move this forward

Though as noted before ( conda-forge/llvmdev-feedstock#192 (comment) ) my time is somewhat limited at present (so 1 day or less turnarounds are unrealistic)

The concern raised in this comment still stands: #72 (comment)

Namely we could wind up with an LLVM installed that doesn't have SVML support while llvmlite does and that could have a negative user experience

Would suggest thinking about how to mitigate that a bit more

@xaleryb
Copy link
Author

xaleryb commented Aug 10, 2023

@jakirkham generally speaking your concern can be valid in some cases but this PR is about different - we are trying to resolve situation which is completely blocking SVML usage for now. By merging this PR users can be unblocked (like in our situation).
Next step - to think how resolve situation you mentioned - per my understanding can be resolved by correct pinning in recipe. If changes (SVML enabling) required rebuild of llvmlite (changes are breaking ABI) version must be incremented but not just a build number. And correct pinning (in meta.yaml) to this version will resolve problem you describing.
Does it make sense? or there is something I missed?

@jakirkham
Copy link
Member

Sorry, but would disagree that it is a separate concern

Before this PR, users don't have a risk of getting an llvm package that is incompatible with the llvmlite package. After this PR, that risk exists. In addition this incompatibility can lead to wrong results or crashes ( #72 (comment) ). So this would be a new bug that is introduced in the attempt to provide a performance improvement in some cases

Would instead posit that introducing a bug is worse behavior from a user perspective. If we desire to have the performance improvement (which is understandable), it should be done in a way that avoids the bug

There are probably several ways that could be done, but some might be constraining the llvm package that is gotten (or the ones avoided) to ensure that only the patched llvm is picked up

@Hardcode84
Copy link

The proper way to fix this will be to link llvm statically.

@jakirkham
Copy link
Member

The proper way would be to upstream the changes ( #72 (comment) )

@Hardcode84
Copy link

Hardcode84 commented Aug 14, 2023

Even if we upstream this (we are working on it, BTW), it will change nothing for this particular version as no one going to update old llvm release. And linking statically helps to avoid a lot more potential issues than this specific SVML one (see my comment about llvm conflict with system runtimes).

@xhochy
Copy link
Member

xhochy commented Aug 15, 2023

Which system runtimes? If we need to link LLVM here statically, we should have to do this everywhere in conda-forge.

@Hardcode84
Copy link

Every single gpu runtime/shader compiler is using llvm under the hood nowadays. The only reason things are still working because they are usually linked statically.

usually

And yes, I would prefer everyone to link llvm statically unless you have a very strong justifications to do otherwise (and distribution size is not one of them).

@xhochy
Copy link
Member

xhochy commented Aug 15, 2023

I think distribution size is a valid argument. Given the amount of LLVM usages from mainline releases, we would otherwise bloat our environments.

@jakirkham
Copy link
Member

Users have been working with Numba on CUDA for some time without issues and this uses the dynamic linking solution we currently have

Not following the argument for static linking

As already noted upthread there are other options for this rebuild other than static linking (like constraining the llvm package)

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.

5 participants