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

Package Flight SQL #793

Merged
merged 1 commit into from
Jul 28, 2022
Merged

Package Flight SQL #793

merged 1 commit into from
Jul 28, 2022

Conversation

lidavidm
Copy link
Contributor

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.

Enable the Flight SQL build. This doesn't have any additional dependencies.

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

@lidavidm
Copy link
Contributor Author

Hmm, I'm going to have to debug that Windows issue (and figure out why our existing CI doesn't catch it). Do we prefer dropping Flight SQL from Windows and proceeding for now, or just holding off with this until 9.0.0 (assuming I get in a fix)?

@xhochy
Copy link
Member

xhochy commented Jun 23, 2022

If the fix is small, we can also apply a patch on top of 8.0.0 here.

@lidavidm lidavidm force-pushed the flight-sql branch 2 times, most recently from f4244e7 to 54a6e8d Compare June 27, 2022 13:27
@lidavidm
Copy link
Contributor Author

Looks like one build flaked. I disabled it on Windows since it's nontrivial to fix (apache/arrow#13434)

@lidavidm lidavidm force-pushed the flight-sql branch 2 times, most recently from c9af988 to 232d628 Compare June 30, 2022 20:18
@lidavidm
Copy link
Contributor Author

lidavidm commented Jul 7, 2022

Looks like some of the builds tend to time out (maybe adding Flight SQL is pushing us above the threshold) - wonder if there's some way to speed up the builds or trim things we don't need. I know some of the Arrow CI builds enable unity builds to try to speed things up, maybe that's worth trying here?

@kkraus14
Copy link
Contributor

Looks like some of the builds tend to time out (maybe adding Flight SQL is pushing us above the threshold) - wonder if there's some way to speed up the builds or trim things we don't need. I know some of the Arrow CI builds enable unity builds to try to speed things up, maybe that's worth trying here?

I tried requeueing but we're hitting timeouts on emulated builds here. We should probably try to move towards cross compilation but that's a relatively big lift.

@jakirkham
Copy link
Member

Unfortunately cross-compiling and CUDA builds don't work together today.

@kkraus14
Copy link
Contributor

Unfortunately cross-compiling and CUDA builds don't work together today.

Doesn't that only apply to device code requiring nvcc? The arrow package just uses the CUDA driver API without any actual device code. We should be able to cross compile against a libcuda stub?

@jakirkham
Copy link
Member

Which means we need to use the CUDA Docker images. This is part of the issue.

@kkraus14
Copy link
Contributor

Any issues with merging this without all of the emulated builds passing? I can then babysit the merge commit jobs to make sure we get packages published.

@jakirkham
Copy link
Member

None. Though it looks like we are down to one.

@kkraus14
Copy link
Contributor

None. Though it looks like we are down to one.

Will give this a chance to finish its run, but if it fails lets merge.

@jakirkham jakirkham added the automerge Merge the PR when CI passes label Jul 28, 2022
@github-actions github-actions bot merged commit f20801a into conda-forge:main Jul 28, 2022
@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
  • azure: passed

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

@lidavidm
Copy link
Contributor Author

Thanks everyone!

@h-vetinari
Copy link
Member

h-vetinari commented Sep 15, 2022

Unfortunately cross-compiling and CUDA builds don't work together today.

Doesn't that only apply to device code requiring nvcc? The arrow package just uses the CUDA driver API without any actual device code. We should be able to cross compile against a libcuda stub?

Which means we need to use the CUDA Docker images. This is part of the issue.

I think I figured out the images part (or at least the rendering part) in #859. Meaning we have x86 build compilers, but I don't yet know how we get the libcuda stub for host (aarch/ppc) into there. We might be able to download it in the build scripts...?

@kkraus14
Copy link
Contributor

We might be able to download it in the build scripts...?

You're not allowed to redistribute a libcuda stub in a container unless the container was based on the nvidia/cuda container.

@h-vetinari
Copy link
Member

You're not allowed to redistribute a libcuda stub in a container unless the container was based on the nvidia/cuda container.

I propose to continue the discussion in #859, if that's alright.

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.

6 participants