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

[r] Use arrow via Suggests: only easing the build requirements #2415

Closed
wants to merge 1 commit into from

Conversation

eddelbuettel
Copy link
Contributor

Issue and/or context:

Conda build have issues with Arrow at the moment.

Changes:

Demote arrow to Suggests:

Notes for Reviewer:

SC 44856

Copy link
Member

@johnkerl johnkerl left a comment

Choose a reason for hiding this comment

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

Flagging as "request changes" only to put merges on pause/freeze, in case anyone else approves & merges -- I'll address
TileDB-Inc/tiledbsoma-feedstock#126 (comment)
momentarily which will try a feedstock build, pointed at this PR's commit hash 🤞

@jdblischak
Copy link
Collaborator

Latest updates:

Now to review this PR:

  • Moving {arrow} to Suggests would indeed bypass this error since {arrow} wouldn't be loaded during either the build or the import test
  • While {arrow} is no longer integral to r-tiledbsoma because of {nanoarrow}, I still found a decent number of instances of arrow::, especially in SOMAExperimentAxisQuery.R. Even if we created a helper function such as hasArrow <- function() return(requireNamespace("arrow", quietly = TRUE)), checking if {arrow} is installed would add some clutter to the code base.
  • Even if we avoided loading {arrow} during the r-tiledbsoma conda build, the overall build would still presumably fail when tiledbsoma-py imports pyarrow during the import test (__init__.py -> from ._dataframe import DataFrame -> _dataframe.py - > import pyarrow as pa). I can't actually confirm this (now that the repodata has been patched, I can't co-install pyarrow with snappy 1.2 to test), so it's possible that pyarrow delays reading the symbols in libarrow.so until they are needed

Given all of the above, I don't have strong feelings about this PR since it won't affect the feedstock builds much. If we decide to go this route, I think we should first protect all the arrow:: calls with requireNamespace(). And I can update the recipe to remove r-arrow. But I'm also fine with closing this PR.

@eddelbuettel
Copy link
Contributor Author

eddelbuettel commented Apr 10, 2024

I concur. The PR was a first 'fast and narrow' attempt to unblock (when the real step was of course to prevent the bad snappy version) and submitted it to review by Aaron and Paul: We can or could condition arrow, if we want to fully is a higher-level decision because two things are simultaneously true: a) arrow is finicky and we all sleep better when we rely less on it on the critical path yet b) we made it somewhat central to the functioning of the package so the drivers of that use need to make the call about relaxing arrow or not. I can see both sides, really.

Your second bullet point, though, may not be as bad. I have by now a bit of experience with nanoarrow on the R side. Based on that (which arguably could be way off given what little I know about Python internals of Arrow and nanoarrow) I can assure you that nanoarrow ("the package") is in fact zero depends on build and installation but will happily farm out to Arrow for some operations under the hood. But "merely having arrow installed" is a much lesser burden than forcing it as a package build dependency. I would think the same is attempted on the Python side. If it is already achieved as it has been on the R side I cannot tell.

@johnkerl
Copy link
Member

#2414

[sc-44854]

@johnkerl
Copy link
Member

As discussed above I believe we can close this PR.

if we need to re-open please either re-open this PR or create a new one.

@johnkerl johnkerl closed this Apr 11, 2024
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.

3 participants