snowflake-connector-python v3.0.0 #125
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nda-forge-pinning 2023.01.28.02.34.29
regro-cf-autotick-bot
requested review from
fhoehle,
hajapy,
mariusvniekerk,
Talues and
xhochy
as code owners
|
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 ( |
|
snowflakedb/snowflake-connector-python@91ec838 seems to switch to using c++17 |
|
Here's the error I saw from build logs before it gets deleted that prompted the revert of std::string_view |
|
A bunch of the failures are now: Looks like we already use SF_ARROW_LIBDIR to tell it where to look, but still it cannot find it? Hmm. Edit: added a debug to show what lib it couldn't find: |
|
Hmm, I checked inside a published pyarrow archive and do not see a libarrow_python.so file. |
|
Looks like we might need patches that depend on arrow version and also looks like the pyarrow build is missing so objects for arrow versions less than 10. I'll open an issue at https://github.com/conda-forge/arrow-cpp-feedstock |
1 task
|
It looks like the wrong variants of pyarrow are being installed for different architectures. This will also need a patch to switch between std::string_view and arrow::util::string_view for different arrow versions. I have to put this down for now, but I made some progress. If someone wants to pick it up, would be happy to have the assistance. |
|
It's compiling better now but running into issues finding the needed DSO (all arrow 10 builds) or segfaulting due to missing symbols (all linux builds except arrow 10). Example from linux_64_arrow_cpp10.0.1numpy1.20python3.8.____cpython failure Then in the import test: linux_aarch64_arrow_cpp9.0.0numpy1.23python3.11.____cpython demangled symbol name: |
|
@conda-forge/arrow-cpp do you have any ideas how to proceed here or what might be wrong? |
This is because there were substantial changes in the library setup in arrow v10 (and all following versions) - the core This new version was then migrated (I commented in the bot PR at the time), but then the migration was closed, meaning that conda-forge by default will now build It's possible that to the build section. |
The connector only supports arrow 10 as of v3. Previously, it only supported arrow 8, though in CF we built it for multiple versions. Now we also have to patch the connector's cpp code to make it work for arrow 7, 8, 9, and 10 which is a new level of patching needed on this recipe and increases the support burden.
I will try this! |
|
@hajapy any luck on getting this build working? Really need v3.0.0 |
|
I don't really have any more ideas how to fix it. Welcome to suggestions or contributions from others. |
h-vetinari
reviewed
Feb 23, 2023
Comment on lines
+109
to
+110
| + if pyarrow_version[0] < 10: | ||
| + std_version = "c++11" |
There was a problem hiding this comment.
This is wrong in conda-forge. Everything is built with C++17
There was a problem hiding this comment.
I'll try to drop this after adjusting the rpaths. I think I was getting compiler errors for older arrow versions which prompted me to mirror how the upstream lib configured their build before they upgraded to arrow 10.
Comment on lines
56
to
+66
| arrow_libs_to_copy = { | ||
| - "linux": ["libarrow.so.800", "libarrow_python.so.800", "libparquet.so.800"], | ||
| + "linux": ["libarrow.so", "libarrow_python.so", "libparquet.so"], | ||
| "linux": [ | ||
| - "libarrow.so.1000", | ||
| - "libarrow_dataset.so.1000", | ||
| - "libarrow_python.so.1000", | ||
| - "libparquet.so.1000", | ||
| + "libarrow.so", | ||
| + "libarrow_dataset.so", | ||
| + "libarrow_python.so", | ||
| + "libparquet.so", | ||
| ], |
There was a problem hiding this comment.
There are several things wrong with this:
- no copying of libs should happen (might be necessary for wheels, but nono for conda-forge)
- you're copying the symlinks (pointing to the versioned libs) without copying what the link points to.
There was a problem hiding this comment.
IIUC, we're setting SF_NO_COPY_ARROW_LIB=1 to disable the actual copying, this patch just allows for building against multiple versions of arrow.
IMO, as a maintainer it would be easier if we only needed to support one version of arrow to match what the upstream library actually supports. This is done so we can support many versions of arrow.
There was a problem hiding this comment.
IMO, as a maintainer it would be easier if we only needed to support one version of arrow to match what the upstream library actually supports. This is done so we can support many versions of arrow.
Supporting only one version of arrow is convenient but very user-hostile. If snowflake pinned to 9 and another package pinned to 10, there wouldn't be a way to co-install them. Hard pins should really be a last resort, and with the exception of the library restructuring with arrow 10, there shouldn't be much of an issue to support several arrow versions.
The alternative is either user pain or a real degradation in usability of the snowflake connector.
There was a problem hiding this comment.
Understood. Prior to this there were no actual library differences needed but now it seems we will need to maintain a patch to keep the backwards compatibility for arrow <10. Hopefully that won't be too long lived.
recipe/meta.yaml
Outdated
| @@ -27,6 +27,9 @@ build: | |||
| - snowflake-dump-ocsp-response-cache = snowflake.connector.tool.dump_ocsp_response_cache:main | |||
| - snowflake-dump-certs = snowflake.connector.tool.dump_certs:main | |||
| - snowflake-export-certs = snowflake.connector.tool.export_certs:main | |||
| rpaths: | |||
| - lib/ | |||
| - $SP_DIR/pyarrow | |||
There was a problem hiding this comment.
I'm sorry I gave imprecise instructions initially. Please use:
Suggested change
| - $SP_DIR/pyarrow | |
| - {{ SP_DIR }}/pyarrow |
There was a problem hiding this comment.
There are still runtime segfaults on Linux despite this 🤔
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It is very likely that the current package version for this feedstock is out of date.
Checklist before merging this PR:
license_fileis packagedInformation about this PR:
@conda-forge-admin,please add bot automergein the title and merge the resulting PR. This command will add our bot automerge feature to your feedstock.bot-rerunlabel to this PR. The bot will close this PR and schedule another one. If you do not have permissions to add this label, you can use the phrase@conda-forge-admin, please rerun botin a PR comment to have theconda-forge-adminadd it for you.Closes: #124
Dependency Analysis
We couldn't run dependency analysis due to an internal error in the bot. :/ Help is very welcome!
This PR was created by the regro-cf-autotick-bot. The regro-cf-autotick-bot is a service to automatically track the dependency graph, migrate packages, and propose package version updates for conda-forge. Feel free to drop us a line if there are any issues! This PR was generated by https://github.com/regro/autotick-bot/actions/runs/4029676573, please use this URL for debugging.