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

Enable CUDA support [READY] #125

Merged
merged 38 commits into from
Apr 28, 2020
Merged

Conversation

pearu
Copy link
Contributor

@pearu pearu commented Mar 19, 2020

Checklist

  • Used a fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • 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.

@conda-forge-linter
Copy link
Contributor

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.

I do have some suggestions for making it better though...

For recipe:

  • It looks like the 'arrow-cpp' output doesn't have any tests.

@xhochy xhochy requested a review from isuruf March 19, 2020 13:54
@pearu pearu mentioned this pull request Mar 19, 2020
4 tasks
@conda-forge-linter
Copy link
Contributor

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

I wanted to let you know that I linted all conda-recipes in your PR (recipe) and found some lint.

Here's what I've got...

For recipe:

  • The recipe must have some tests.
  • The recipe must have a build/number section.

For recipe:

  • License is not an SPDX identifier (or Other) nor an SPDX license expression.

@isuruf
Copy link
Member

isuruf commented Mar 19, 2020

@pearu, your idea is a good one, but this is how you should implement it.
Skip linux64 and cuda_compiler_version != 9.2. (That means you only build with cuda 9.2 on linux64.) Have two packages arrow-cpp and arrow-cpp-cuda.

@pearu
Copy link
Contributor Author

pearu commented Mar 19, 2020

@isuruf ok, I'll fix to using 9.2. Two questions:

  1. How to fix the following osx CI failure:
Error: Failed to render jinja template in /Users/runner/runners/2.165.2/work/1/s/recipe/meta.yaml:
'cuda_compiler_version' is undefined
  1. When building arrow-cpp against cuda 9.2, will the conda package still be usable in environments with cuda > 9.2? Why not build against the latest cuda version?

@isuruf
Copy link
Member

isuruf commented Mar 19, 2020

When building arrow-cpp against cuda 9.2, will the conda package still be usable in environments with cuda > 9.2?

Yes, it looks like arrow is using only the CUDA driver API. @xhochy should confirm.

@conda-forge-linter
Copy link
Contributor

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.

I do have some suggestions for making it better though...

For recipe:

  • License is not an SPDX identifier (or Other) nor an SPDX license expression.

@pearu
Copy link
Contributor Author

pearu commented Mar 19, 2020

@conda-forge-admin, please rerender

@jakirkham
Copy link
Member

Other packages are using a mutex package.

@isuruf
Copy link
Member

isuruf commented Apr 16, 2020

It seems like we can always change which way we are doing things if we want to in the future. arrow-cpp=*=*cuda seems cleaner to the user to me, but again, it doesn't really matter.

It's hard to change stuff in the future. Get it right first, otherwise people will complain.

That said, we can have both. We can have the build string in it and users who want to use the mutex package can do so.

We should also have a look at the name. arrow-cpp-build-ext, ucx-proc, etc.

@jakirkham
Copy link
Member

jakirkham commented Apr 16, 2020

No objections to keeping both.

Edit: Agree it would be nice to have the name align. Other packages currently do *-proc.

@pearu pearu requested a review from pcmoritz as a code owner April 17, 2020 07:51
@pearu
Copy link
Contributor Author

pearu commented Apr 17, 2020

I have fixed merged conflicts and renamed arrow-cpp-build-ext to arrow-cpp-proc.

@isuruf @jakirkham please review.

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Looks good @pearu! Thanks again for working on this 😄

Had some suggestions below that might simplify the syntax a bit and drop some things we don't need. Nothing critical though.

recipe/meta.yaml Outdated Show resolved Hide resolved
recipe/meta.yaml Outdated Show resolved Hide resolved
recipe/meta.yaml Outdated Show resolved Hide resolved
recipe/meta.yaml Outdated Show resolved Hide resolved
recipe/meta.yaml Outdated Show resolved Hide resolved
@pearu pearu requested review from jakirkham and isuruf April 18, 2020 19:09
@pearu pearu changed the title Enable CUDA support - 2nd try. Enable CUDA support [READY] Apr 20, 2020
@pearu
Copy link
Contributor Author

pearu commented Apr 21, 2020

Hi all, could we have this PR merged?

@pearu
Copy link
Contributor Author

pearu commented Apr 22, 2020

Several projects using CUDA rely on arrow-cpp 0.16 and packaging those are currently blocked by this PR.
@xhochy could you merge this PR?

@xhochy
Copy link
Member

xhochy commented Apr 22, 2020

@pearu Leaving this open until tomorrow to let @isuruf give any objections, otherwise I'm going to have a review by myself then (no time for that today). I will merge this to a separated 0.16 branch and will port things over to 0.17 afterwards.

@jakirkham
Copy link
Member

LGTM. Thanks for your work on this @pearu! 😄

@alexbaden
Copy link

Hi all -- thanks for doing this work @pearu and to everyone else for reviewing. We just pushed out a new version of OmniSciDB which uses Arrow 0.16 and are interested in seeing this patch merged so we can push our python connector changes as well. Is there a rough ETA?

@xhochy xhochy changed the base branch from master to 0.16.x April 27, 2020 11:48
@jakirkham
Copy link
Member

FWIW this issue may be relevant.

xref: https://issues.apache.org/jira/browse/ARROW-8608

@xhochy xhochy merged commit 9129ea2 into conda-forge:0.16.x Apr 28, 2020
@pearu pearu deleted the pearu/enable-cuda-2 branch April 30, 2020 12:18
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.

8 participants