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

RFC: Rename arrow-cpp to libarrow #158

Closed
wants to merge 5 commits into from

Conversation

h-vetinari
Copy link
Member

Since the discussion started here, I'm making this PR for a first stab at an approach to reuse for conda-forge/conda-forge.github.io#1073 more generally.

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

@h-vetinari h-vetinari marked this pull request as draft June 27, 2020 09:53
Comment on lines +19 to 22
# for cuda, building with 9.2 is enough to be compatible with all later versions,
# since arrow is only using libcuda, and not libcudart.
skip: true # [cuda_compiler_version not in (undefined, "None", "9.2")]
Copy link
Member Author

Choose a reason for hiding this comment

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

These comments are independent from the rest of the PR, but I thought I'd through them in nevertheless.

I had gotten this aspect wrong myself at first, and was pointed to the discussions in #123 & #125. This is my best attempt at destilling the information into a short comment that might help another future reader of this recipe to figure out what's happening.

@pearu
Copy link
Contributor

pearu commented Jun 27, 2020

The rename sounds a big change to be accomplished by just via build number increase: although the change is minimal here, it will affect many other packages.

To reduce the burden on the affected package maintenance, would it make sense to introduce a new conda-forge libarrow-feedstock that implements the renaming? When done,

  • the affected packages can switch to libarrow, or not (in short-term),
  • arrow-cpp references libarrow and it will be archived eventually.

@h-vetinari
Copy link
Member Author

h-vetinari commented Jun 27, 2020

@pearu: The rename sounds a big change to be accomplished by just via build number increase: although the change is minimal here, it will affect many other packages.

Other recipes depending on arrow would not be affected; this is the reason why I'm keeping the old output names (which are wired under the hood to be exactly the same as before).

This would leave some time to migrate those other recipes away from arrow-cpp towards libarrow, and then eventually remove the arrow-cpp output here.

All the feedstock-based operations are IMO much higher in terms of effort / complexity / possible disruption.

@pearu
Copy link
Contributor

pearu commented Jun 27, 2020

All the feedstock-based operations are IMO much higher in terms of effort / complexity / possible disruption.

ok, it makes sense.

Just FYI, I'd like to backport arrow-cpp / libarrow to 0.16 in the relatively near future to resolve conda-forge/omniscidb-feedstock#7.

@h-vetinari
Copy link
Member Author

Just FYI, I'd like to backport arrow-cpp / libarrow to 0.16 in the relatively near future to resolve conda-forge/omniscidb-feedstock#7.

Not sure I understand what you want to backport. Do you mean the naming? If yes, migrating dependent packages to the new naming will have several months of time to play out (or even more), so if you need to fix something now, just use what's there already, i.e. arrow-cpp.

@pearu
Copy link
Contributor

pearu commented Jun 27, 2020

By backport, I mean updating https://github.com/conda-forge/arrow-cpp-feedstock/tree/0.16.x according to the current arrow-cpp and applying the arrow patch to resolve the issue conda-forge/omniscidb-feedstock#7 .

recipe/meta.yaml Outdated Show resolved Hide resolved
recipe/meta.yaml Outdated Show resolved Hide resolved
@@ -36,7 +38,17 @@ outputs:
- LICENSE.txt
summary: 'A meta-package to select Arrow build variant'

- name: arrow-cpp
# compat output for old mutex-package naming
- name: arrow-cpp-proc
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this have a run requirement for the new -proc package?

Copy link
Member Author

Choose a reason for hiding this comment

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

True

Also adapt the main package name, as apparently, it doesn't have to
match the feedstock name once the latter has been created.
Copy link
Member Author

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

Sorry this was lying around for a while

@@ -36,7 +38,17 @@ outputs:
- LICENSE.txt
summary: 'A meta-package to select Arrow build variant'

- name: arrow-cpp
# compat output for old mutex-package naming
- name: arrow-cpp-proc
Copy link
Member Author

Choose a reason for hiding this comment

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

True

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

h-vetinari commented Dec 29, 2020

@xhochy
Does arrow-cpp really need to depend on python & numpy? Doesn't that kind of obviate a large portion of the benefits of separating the cpp-part from pyarrow?

(BTW, not entirely sure why conda decided to rerender the feedstock as building all python versions in one CI job, but that's mostly a separate issue - aside from leading me to the question why libarrow is built several times, which has to do with the python dependency)

@xhochy
Copy link
Member

xhochy commented Dec 29, 2020

@xhochy
Does arrow-cpp really need to depend on python & numpy? Doesn't that kind of obviate a large portion of the benefits of separating the cpp-part from pyarrow?

Yes, it does currently. I would delay the renaming until we can split libarrow and libarrow_python into separate outputs. This will then probably still be a build with a full Python matrix but at least the libarrow output would then be Python independent.

This needs some changes to the build upstream first and would otherwise lead to quite some patching in the feedstock otherwise.

@h-vetinari
Copy link
Member Author

@xhochy: This needs some changes to the build upstream first and would otherwise lead to quite some patching in the feedstock otherwise.

Is there a JIRA ticket for this already? If I knew more or less what needs to be done, it might be feasible to develop the patches in a PR on the feedstock and upstream them.

@xhochy
Copy link
Member

xhochy commented Jan 10, 2021

@h-vetinari There is https://issues.apache.org/jira/browse/ARROW-8518 where also a Draft PR is linked that tries to separate Gandiva out.

@h-vetinari
Copy link
Member Author

Thanks for the link. You mentioned "changes [plural] to the build upstream" - what else would be necessary? Would it make sense to include apache/arrow#8390 here as a test?

@h-vetinari
Copy link
Member Author

Does arrow-cpp really need to depend on python & numpy? Doesn't that kind of obviate a large portion of the benefits of separating the cpp-part from pyarrow?

Yes, it does currently. I would delay the renaming until we can split libarrow and libarrow_python into separate outputs. This will then probably still be a build with a full Python matrix but at least the libarrow output would then be Python independent.

Blast from the past here @xhochy, this seems to be getting ready for 10.0 (see #862). 🥳

(The independent output aspect for flight/gandiva etc. seems to have stalled however, but that's independent of the lib rename IMO).

h-vetinari added a commit to regro-cf-autotick-bot/arrow-cpp-feedstock that referenced this pull request Nov 7, 2022
h-vetinari added a commit to regro-cf-autotick-bot/arrow-cpp-feedstock that referenced this pull request Nov 7, 2022
@h-vetinari h-vetinari mentioned this pull request Nov 7, 2022
3 tasks
h-vetinari added a commit to regro-cf-autotick-bot/arrow-cpp-feedstock that referenced this pull request Nov 7, 2022
h-vetinari added a commit to regro-cf-autotick-bot/arrow-cpp-feedstock that referenced this pull request Nov 7, 2022
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.

4 participants