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

Add CUDA build #101

Merged
merged 39 commits into from
May 20, 2020
Merged

Conversation

pearu
Copy link

@pearu pearu commented Mar 25, 2020

This PR requires conda-forge/arrow-cpp-feedstock#125

Checklist

  • Used a 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.

@conda-forge-linter
Copy link

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:

  • Failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your meta.yaml, though. To get a traceback to help figure out what's going on, install conda-smithy and run conda smithy recipe-lint . from the recipe directory.

@conda-forge-linter
Copy link

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.

@conda-forge-linter
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.

@pearu pearu requested a review from pcmoritz as a code owner April 17, 2020 08:58
@xhochy xhochy changed the base branch from master to 0.16.x April 27, 2020 12:31
host:
# directly pin boost-cpp as we also seem to directly include boost symbols
# in the Python modules.
- arrow-cpp {{ version }}
- arrow-cpp ={{ version }}=*{{ build_ext }}
Copy link
Author

Choose a reason for hiding this comment

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

@h-vetinari , arrow-cpp-proc in run_constraint ought to handle the matching with build_ext.

Copy link
Member

Choose a reason for hiding this comment

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

I know, but the same information is in the build-string of arrow-cpp, making it wholly unnecessary to depend on the version (and build string) of arrow-cpp-proc. If you don't like it, then feel free to revert - I consider it cleaner/simpler.

Copy link
Member

Choose a reason for hiding this comment

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

This notation is probably the preferred one for conda.

Suggested change
- arrow-cpp ={{ version }}=*{{ build_ext }}
- arrow-cpp {{ version }} *{{ build_ext }}

Copy link
Member

Choose a reason for hiding this comment

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

@pearu The run_constraint will only be applied if arrow-cpp-proc is installed. Currently it seems to me that when you don't have arrow-cpp-proc installed, you could install the cpu version of arrow-cpp while installing the CUDA version of pyarrow.

Copy link
Member

Choose a reason for hiding this comment

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

This notation is probably the preferred one for conda.

Personally I dislike significant whitespace for this - I think it's very hard to read.

Currently it seems to me that when you don't have arrow-cpp-proc installed, you could install the cpu version of arrow-cpp while installing the CUDA version of pyarrow.

That's IMO not possible because we're depending on the specific build version of arrow-cpp already.

Copy link
Member

Choose a reason for hiding this comment

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

This notation is probably the preferred one for conda.

Personally I dislike significant whitespace for this - I think it's very hard to read.

I didn't make the standard but I like to keep it :(

Currently it seems to me that when you don't have arrow-cpp-proc installed, you could install the cpu version of arrow-cpp while installing the CUDA version of pyarrow.

That's IMO not possible because we're depending on the specific build version of arrow-cpp already.

The comment was meant in regard to not having arrow-cpp ={{ version }}=*{{ build_ext }} in run. From my understanding, if that would be omitted, the mentioned situation could happen (or I'm missing another safeguard?)

Copy link
Member

Choose a reason for hiding this comment

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

The comment was meant in regard to not having arrow-cpp ={{ version }}=*{{ build_ext }} in run. From my understanding, if that would be omitted, the mentioned situation could happen (or I'm missing another safeguard?)

No you're right, but not having any requirement on arrow-cpp in run broke the build, as both @pearu and I had to find out. So I don't think it can be omitted anyway, and then why shouldn't we also add the build_ext?

I didn't make the standard but I like to keep it :(

It's not a hill I'm gonna die on, but that's IMO a bad reason to write hard-to-read recipes (and it's not like the variants are so different as to be unrecognisable).

@pearu
Copy link
Author

pearu commented May 15, 2020

Nothing I touched should have affected the osx/gandiva builds, but there's an error

import: 'pyarrow.gandiva'

CUDA enabled arrow-cpp 0.16 disables gandiva for ppc64le and aarch64, see https://github.com/conda-forge/arrow-cpp-feedstock/blob/0.16.x/recipe/build-arrow.sh#L11

CUDA enabled arrow-cpp 0.17, however, will enable gandiva support, see conda-forge/arrow-cpp-feedstock#137 .

However, for this PR, that is arrow-cpp 0.16 based, the gandiva support must be disabled.

@pearu
Copy link
Author

pearu commented May 15, 2020

Same error on linux64 (but not on aarch/ppc):

import: 'pyarrow.gandiva'
...
ImportError: libre2.so.6: cannot open shared object file: No such file or directory

The latest re2 package (v 2020.05) provides libre2.so.7 but arrow-cpp 0.16 was built against re2 package v 2020.04 that provides libre2.so.6.

@pearu
Copy link
Author

pearu commented May 15, 2020

This PR is blocked by conda-forge/conda-forge-ci-setup-feedstock#93 .

@xhochy
Copy link
Member

xhochy commented May 16, 2020

Same error on linux64 (but not on aarch/ppc):

import: 'pyarrow.gandiva'
...
ImportError: libre2.so.6: cannot open shared object file: No such file or directory

The latest re2 package (v 2020.05) provides libre2.so.7 but arrow-cpp 0.16 was built against re2 package v 2020.04 that provides libre2.so.6.

Made a hotfix PR: conda-forge/admin-requests#53

recipe/meta.yaml Outdated Show resolved Hide resolved
@h-vetinari
Copy link
Member

@conda-forge-admin, please restart ci

@h-vetinari
Copy link
Member

@pearu @xhochy
The GPU builds are green now 🥳
Just waiting for aarch/ppc to finish.

recipe/meta.yaml Outdated Show resolved Hide resolved
@pearu
Copy link
Author

pearu commented May 20, 2020

@h-vetinari @xhochy @isuruf , are there any issues left that prevent landing this PR?

@xhochy xhochy closed this May 20, 2020
@xhochy xhochy reopened this May 20, 2020
@@ -12,7 +15,11 @@ source:
sha256: {{ checksum }}

build:
number: 2
number: {{ number }}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
number: {{ number }}
number: 3

@@ -1,6 +1,9 @@
{% set version = "0.16.0" %}
{% set filename = "apache-arrow-" + version + ".tar.gz" %}
{% set checksum = "261992de4029a1593195ff4000501503bd403146471b3168bd2cc414ad0fb7f5" %}
{% set number = "3" %}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{% set number = "3" %}

@@ -12,7 +15,11 @@ source:
sha256: {{ checksum }}

build:
number: 2
number: {{ number }}
string: "py{{ CONDA_PY }}h{{ PKG_HASH }}_{{ number }}_{{ build_ext }}"
Copy link
Member

Choose a reason for hiding this comment

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

@xhochy
The number is also necessary for the build string. While I'm the first to be against useless templating, I think having {{ number }} makes a lot of sense in this recipe.

@xhochy
Copy link
Member

xhochy commented May 20, 2020

Ignore the suggestions, if CI passes again, we can merge.

@xhochy xhochy added the automerge Merge the PR when CI passes label May 20, 2020
@github-actions github-actions bot merged commit 5d1ee02 into conda-forge:0.16.x May 20, 2020
@github-actions
Copy link
Contributor

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

  • linter: passed
  • travis: passed
  • azure: passed

Thus the PR was passing and merged! Have a great day!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge the PR when CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants