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 support for master #137

Merged
merged 6 commits into from
May 18, 2020
Merged

Add cuda support for master #137

merged 6 commits into from
May 18, 2020

Conversation

xhochy
Copy link
Member

@xhochy xhochy commented May 14, 2020

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
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:

  • Jinja2 variable references are suggested to take a {{<one space><variable name><one space>}} form. See lines [16].

@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.

@xhochy
Copy link
Member Author

xhochy commented May 14, 2020

@pearu I took a stab at copying over the CUDA changes to 0.17/master, can you take a look?

Copy link
Contributor

@pearu pearu left a comment

Choose a reason for hiding this comment

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

It seems that {{ compiler("cuda") }} is missed in build dependencies.

I am not sure why the cuda enabled build was successful now. I guess, after re-rendering the recipe, it will break.

- ninja
- make # [unix]
- {{ compiler('c') }}
- {{ compiler('cxx') }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the following dependency is required in build:

    - {{ compiler("cuda") }}  # [cuda_compiler_version is not undefined and cuda_compiler_version
 == '9.2']

@xhochy
Copy link
Member Author

xhochy commented May 15, 2020

@conda-forge-admin please rerender

@pearu
Copy link
Contributor

pearu commented May 15, 2020

Re failing win builds: the build log shows

2020-05-15T07:53:41.8464152Z Traceback (most recent call last):
2020-05-15T07:53:41.8464746Z   File "C:\Miniconda\lib\site-packages\conda\gateways\disk\create.py", line 245, in make_menu
2020-05-15T07:53:41.8465293Z     import menuinst
2020-05-15T07:53:41.8465780Z   File "C:\Miniconda\lib\site-packages\menuinst\__init__.py", line 23, in <module>
2020-05-15T07:53:41.8466306Z     from .win32 import Menu, ShortCut
2020-05-15T07:53:41.8466796Z   File "C:\Miniconda\lib\site-packages\menuinst\win32.py", line 19, in <module>
2020-05-15T07:53:41.8467341Z     from .winshortcut import create_shortcut
2020-05-15T07:53:41.8474996Z ModuleNotFoundError: No module named 'menuinst.winshortcut'done

Following conda/conda#1762 (comment), perhaps try adding menuinst to build dependencies?

Another sign of failure is
https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=162703&view=logs&j=5be07ae1-d8ba-5406-47b6-8e3a3a12f825&t=ac199efb-6791-5d5e-770f-2831854227bc&l=12
for which I don't have interpretation. @isuruf ?

Finally, what is the reason for restricting to cmake 3.16?

@xhochy
Copy link
Member Author

xhochy commented May 15, 2020

I suspect that the build script being the default name bld.bat could have been the issue. Let's see what CI tells us.

The reason to pin to cmake 3.16 is that Arrow doesn't detect the correct Python with cmake 3.17, this should be fixed again in 3.18, then we can remove the constraint again.

@xhochy
Copy link
Member Author

xhochy commented May 15, 2020

Seems like renaming helped a lot. I will revert the ppc64le builds again to Travis to get them back to speed.

build:
number: {{ number }}
string: "py{{ CONDA_PY }}h{{ PKG_HASH }}_{{ number }}_{{ build_ext }}"
skip: true # [cuda_compiler_version not in (undefined, "None", "9.2")]
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
skip: true # [cuda_compiler_version not in (undefined, "None", "9.2")]

I think it's worth trying to see whether cuda 10.x builds successfully. 9.2. cannot make use of the compute_75 instruction set, for example.

Copy link
Member

Choose a reason for hiding this comment

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

Following this suggestion would need a few more adaptations (using # [cuda_compiler_version is not undefined and cuda_compiler_version != "None"] as a selector), plus a rerender

Copy link
Member

Choose a reason for hiding this comment

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

That's not necessary. Please read the discussion in the PR that added CUDA support.

Copy link
Member

Choose a reason for hiding this comment

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

OK, thanks for the info. To record the outcome (to the best of my understanding): since arrow only uses the cuda driver (and not the toolkit), building for one cuda version is compatible with all.

@pearu
Copy link
Contributor

pearu commented May 15, 2020

LGTM.

Btw, are there tricks that could avoid the issue describe here that could be applied in this PR to avoid future problems?

@xhochy
Copy link
Member Author

xhochy commented May 18, 2020

LGTM.

Btw, are there tricks that could avoid the issue describe here that could be applied in this PR to avoid future problems?

Yes, we should make better recipes :) @isuruf thankfully already fixed the major breakage and with conda-forge/conda-forge-pinning-feedstock#618 we will have a consistent CF-global linkage with re2.

@xhochy xhochy merged commit 1e57bc0 into conda-forge:master May 18, 2020
@xhochy xhochy deleted the cuda-0.17 branch May 18, 2020 06:09
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