You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "pentschev (via GitHub)" <gi...@apache.org> on 2023/10/20 12:07:24 UTC
[PR] GH-38364: [Python] Initialize S3 on first use [arrow]
pentschev opened a new pull request, #38375:
URL: https://github.com/apache/arrow/pull/38375
### Rationale for this change
In accordance to https://github.com/apache/arrow/issues/38364, we believe that for various reasons (shortening import time, preventing unnecessary resource consumption and potential bugs with S3 library) it is appropriate to avoid initialization of S3 resources at import time and move that step to occur at first-use.
### What changes are included in this PR?
- Remove calls to `ensure_s3_initialized()` that were up until now executed during `import pyarrow.fs`;
- Move `ensure_s3_intialized()` calls to `python/pyarrow/_s3fs.pyx` module;
- Add global flag to mark whether S3 has been previously initialized and `atexit` handlers registered.
### Are these changes tested?
Yes, existing S3 tests check whether it has been initialized, otherwise failing with a C++ exception.
### Are there any user-facing changes?
No, the behavior is now slightly different with S3 initialization not happening immediately after `pyarrow.fs` is imported, but no changes are expected from a user perspective relying on the public API alone.
**This PR contains a "Critical Fix".**
A bug in aws-sdk-cpp reported in https://github.com/aws/aws-sdk-cpp/issues/2681 causes segmentation faults under specific circumstances when Python processes shutdown, specifically observed with Dask+GPUs (so far we were unable to pinpoint the exact correlation of Dask+GPUs+S3). While this definitely doesn't seem to affect all users and is not directly sourced in Arrow, it may affect use cases that are completely independent of S3 to operate, which is particularly problematic in CI where all tests pass successfully but the process crashes at shutdown.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38364: [Python] Initialize S3 on first use [arrow]
Posted by "bdice (via GitHub)" <gi...@apache.org>.
bdice commented on PR #38375:
URL: https://github.com/apache/arrow/pull/38375#issuecomment-1781329550
Yes, thank you @jorisvandenbossche. I agree with @pentschev that our use case for pyarrow will be satisfied without a backport to 14.x, as long as we have the patch in the conda-forge packages for Arrow 13 which was just merged. We'll want the same patch to carry forward to Arrow 14 on conda-forge, until Arrow 15 is released with the fix upstreamed.👍
However, I see this fixed an issue reported by someone else with `r-arrow`, so a backport could be very welcome by other users: https://github.com/rstudio/reticulate/issues/1470#issuecomment-1781242235
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38364: [Python] Initialize S3 on first use [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #38375:
URL: https://github.com/apache/arrow/pull/38375#issuecomment-1777096683
@github-actions crossbow submit -g python -g wheel
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38364: [Python] Initialize S3 on first use [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #38375:
URL: https://github.com/apache/arrow/pull/38375
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38364: [Python] Initialize S3 on first use [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #38375:
URL: https://github.com/apache/arrow/pull/38375#discussion_r1370386844
##########
python/pyarrow/fs.py:
##########
@@ -54,13 +54,12 @@
from pyarrow._s3fs import ( # noqa
AwsDefaultS3RetryStrategy, AwsStandardS3RetryStrategy,
S3FileSystem, S3LogLevel, S3RetryStrategy, ensure_s3_initialized,
- finalize_s3, initialize_s3, resolve_s3_region)
+ finalize_s3, ensure_s3_finalized, initialize_s3, resolve_s3_region)
except ImportError:
_not_imported.append("S3FileSystem")
else:
- ensure_s3_initialized()
import atexit
Review Comment:
```suggestion
# GH-38364: we don't initialize S3 eagerly as that could lead
# to crashes at shutdown even when S3 isn't used.
# Instead, S3 is initialized lazily using `ensure_s3_initialized`
# in assorted places.
import atexit
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38364: [Python] Initialize S3 on first use [arrow]
Posted by "pentschev (via GitHub)" <gi...@apache.org>.
pentschev commented on code in PR #38375:
URL: https://github.com/apache/arrow/pull/38375#discussion_r1369708715
##########
python/pyarrow/_s3fs.pyx:
##########
@@ -260,6 +271,26 @@ cdef class S3FileSystem(FileSystem):
load_frequency=900, proxy_options=None,
allow_bucket_creation=False, allow_bucket_deletion=False,
retry_strategy: S3RetryStrategy = AwsStandardS3RetryStrategy(max_attempts=3)):
+ ensure_s3_initialized()
+
+ self._initialize_s3(access_key=access_key, secret_key=secret_key, session_token=session_token,
+ anonymous=anonymous, region=region, request_timeout=request_timeout,
+ connect_timeout=connect_timeout, scheme=scheme, endpoint_override=endpoint_override,
+ background_writes=background_writes, default_metadata=default_metadata,
+ role_arn=role_arn, session_name=session_name, external_id=external_id,
+ load_frequency=load_frequency, proxy_options=proxy_options,
+ allow_bucket_creation=allow_bucket_creation, allow_bucket_deletion=allow_bucket_deletion,
+ retry_strategy=retry_strategy)
+
+ def _initialize_s3(self, *, access_key=None, secret_key=None, session_token=None,
+ bint anonymous=False, region=None, request_timeout=None,
+ connect_timeout=None, scheme=None, endpoint_override=None,
+ bint background_writes=True, default_metadata=None,
+ role_arn=None, session_name=None, external_id=None,
+ load_frequency=900, proxy_options=None,
+ allow_bucket_creation=False, allow_bucket_deletion=False,
+ retry_strategy: S3RetryStrategy = AwsStandardS3RetryStrategy(max_attempts=3)):
+
Review Comment:
The reason I had to move the implementation to the `_initialize_s3` method are the `cdef`s in https://github.com/apache/arrow/pull/38375/files#diff-afa3ea99a387be221ef1f7230aa309b42001aed318cdc6969e700d5eb04d07b2R294-R296, they will instantiate objects that expect S3 to be already initialized (which is only guaranteed after `ensure_s3_initialized()` is called) and Cython will instantiate them before any code in the function runs, including `ensure_s3_initialized()`.
Another alternative would be to make them `unique_ptr`s or something that won't instantiate the objects immediately at `__init__()`'s entry, but I think this would make the code more complex than the solution currently proposed. If a pointer is preferred here without deferring to another method I can work on that.
##########
python/pyarrow/fs.py:
##########
@@ -57,10 +57,6 @@
finalize_s3, initialize_s3, resolve_s3_region)
except ImportError:
_not_imported.append("S3FileSystem")
-else:
- ensure_s3_initialized()
- import atexit
- atexit.register(finalize_s3)
Review Comment:
That sounds like a good idea, let me try that locally and if tests succeed I'll push the change into this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38364: [Python] Initialize S3 on first use [arrow]
Posted by "rjzamora (via GitHub)" <gi...@apache.org>.
rjzamora commented on PR #38375:
URL: https://github.com/apache/arrow/pull/38375#issuecomment-1776166465
(cc @jorisvandenbossche) We are very interested in feedback on this one. The aws-sdk-cpp bug is causing RAPIDS a lot of pain, and so we are very motivated to avoid the import whenever possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38364: [Python] Initialize S3 on first use [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #38375:
URL: https://github.com/apache/arrow/pull/38375#issuecomment-1779571108
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 7751444be23a433d73d0722d34d5107ca35a714f.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/18048620602) has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38364: [Python] Initialize S3 on first use [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #38375:
URL: https://github.com/apache/arrow/pull/38375#issuecomment-1777498057
@pentschev You're welcome!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38364: [Python] Initialize S3 on first use [arrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on PR #38375:
URL: https://github.com/apache/arrow/pull/38375#issuecomment-1777629546
Thanks all! This is a big help for RAPIDS.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38364: [Python] Initialize S3 on first use [arrow]
Posted by "pentschev (via GitHub)" <gi...@apache.org>.
pentschev commented on PR #38375:
URL: https://github.com/apache/arrow/pull/38375#issuecomment-1772651098
The failed test is a C++ test:
```c++
[ RUN ] TestS3FS.GetFileInfoGeneratorStress
API: DeleteMultipleObjects(bucket=stress, multiObject=true, numberOfObjects=1000)
Time: 12:20:20 UTC 10/20/2023
DeploymentID: 8b5b1ae6-66cf-4ead-a4fd-3ef2b430d16c
RequestID: 178FD01F921FEEC8
RemoteHost: 127.0.0.1
Host: 127.0.0.8:51944
UserAgent: aws-sdk-cpp/1.11.68 Windows/10.0.17763.4252 AMD64 MSVC/1929
Error: remove C:\Users\appveyor\AppData\Local\Temp\1\s3fs-test-30c6atjk/.minio.sys/buckets/stress/1/230/fs.json: The process cannot access the file because it is being used by another process. (*fs.PathError)
6: internal\logger\logger.go:278:logger.LogIf()
5: cmd\fs-v1-helpers.go:430:cmd.fsDeleteFile()
4: cmd\fs-v1.go:1210:cmd.(*FSObjects).DeleteObject()
3: cmd\fs-v1.go:1142:cmd.(*FSObjects).DeleteObjects()
2: cmd\bucket-handlers.go:592:cmd.objectAPIHandlers.DeleteMultipleObjectsHandler()
1: net\http\server.go:2047:http.HandlerFunc.ServeHTTP()
API: DeleteMultipleObjects(bucket=stress, multiObject=true, numberOfObjects=1000)
Time: 12:20:21 UTC 10/20/2023
DeploymentID: 8b5b1ae6-66cf-4ead-a4fd-3ef2b430d16c
RequestID: 178FD01F921FEEC8
RemoteHost: 127.0.0.1
Host: 127.0.0.8:51944
UserAgent: aws-sdk-cpp/1.11.68 Windows/10.0.17763.4252 AMD64 MSVC/1929
Error: remove C:\Users\appveyor\AppData\Local\Temp\1\s3fs-test-30c6atjk/.minio.sys/buckets/stress/1/230/fs.json: The process cannot access the file because it is being used by another process. (*fs.PathError)
5: internal\logger\logger.go:278:logger.LogIf()
4: cmd\api-errors.go:2123:cmd.toAPIErrorCode()
3: cmd\api-errors.go:2148:cmd.toAPIError()
2: cmd\bucket-handlers.go:620:cmd.objectAPIHandlers.DeleteMultipleObjectsHandler()
1: net\http\server.go:2047:http.HandlerFunc.ServeHTTP()
[FATAL] 2023-10-20 12:19:08.379 WinHttpSyncHttpClient [2536] Failed setting secure crypto protocols with error code: 87
[FATAL] 2023-10-20 12:19:08.950 WinHttpSyncHttpClient [2536] Failed setting secure crypto protocols with error code: 87
C:/projects/arrow/cpp/src/arrow/filesystem/s3fs_test.cc(865): error: Failed
'fs_->DeleteDirContents("stress")' failed with IOError: Got the following 1 errors when deleting objects in S3 bucket 'stress':
- key '1/230': We encountered an internal error, please try again.: cause(remove C:\Users\appveyor\AppData\Local\Temp\1\s3fs-test-30c6atjk/.minio.sys/buckets/stress/1/230/fs.json: The process cannot access the file because it is being used by another process.)
[ FAILED ] TestS3FS.GetFileInfoGeneratorStress (72879 ms)
```
This PR is not touching any C++ code, is there any Python code being executed during C++ testing or is it purely a coincidence that the failing test is also an S3 test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38364: [Python] Initialize S3 on first use [arrow]
Posted by "jakirkham (via GitHub)" <gi...@apache.org>.
jakirkham commented on PR #38375:
URL: https://github.com/apache/arrow/pull/38375#issuecomment-1777652998
Thanks all! 🙏
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38364: [Python] Initialize S3 on first use [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #38375:
URL: https://github.com/apache/arrow/pull/38375#discussion_r1369446133
##########
python/pyarrow/fs.py:
##########
@@ -57,10 +57,6 @@
finalize_s3, initialize_s3, resolve_s3_region)
except ImportError:
_not_imported.append("S3FileSystem")
-else:
- ensure_s3_initialized()
- import atexit
- atexit.register(finalize_s3)
Review Comment:
How about using `arrow::EnsureS3Finalized()` instead?
```diff
diff --git a/python/pyarrow/_s3fs.pyx b/python/pyarrow/_s3fs.pyx
index ab4517136..21c3cc94d 100644
--- a/python/pyarrow/_s3fs.pyx
+++ b/python/pyarrow/_s3fs.pyx
@@ -70,6 +70,13 @@ def finalize_s3():
check_status(CFinalizeS3())
+def ensure_s3_finalized():
+ """
+ Finalize S3 if already initialized
+ """
+ check_status(CEnsureS3Finalized())
+
+
def resolve_s3_region(bucket):
"""
Resolve the S3 region of a bucket.
diff --git a/python/pyarrow/fs.py b/python/pyarrow/fs.py
index 36655c7d1..4cb723f53 100644
--- a/python/pyarrow/fs.py
+++ b/python/pyarrow/fs.py
@@ -54,13 +54,12 @@ try:
from pyarrow._s3fs import ( # noqa
AwsDefaultS3RetryStrategy, AwsStandardS3RetryStrategy,
S3FileSystem, S3LogLevel, S3RetryStrategy, ensure_s3_initialized,
- finalize_s3, initialize_s3, resolve_s3_region)
+ finalize_s3, ensure_s3_finalized, initialize_s3, resolve_s3_region)
except ImportError:
_not_imported.append("S3FileSystem")
else:
- ensure_s3_initialized()
import atexit
- atexit.register(finalize_s3)
+ atexit.register(ensure_s3_finalized)
def __getattr__(name):
diff --git a/python/pyarrow/includes/libarrow_fs.pxd b/python/pyarrow/includes/libarrow_fs.pxd
index 2727fc201..cb30f4e75 100644
--- a/python/pyarrow/includes/libarrow_fs.pxd
+++ b/python/pyarrow/includes/libarrow_fs.pxd
@@ -211,6 +211,7 @@ cdef extern from "arrow/filesystem/api.h" namespace "arrow::fs" nogil:
const CS3GlobalOptions& options)
cdef CStatus CEnsureS3Initialized "arrow::fs::EnsureS3Initialized"()
cdef CStatus CFinalizeS3 "arrow::fs::FinalizeS3"()
+ cdef CStatus CEnsureS3Finalized "arrow::fs::EnsureS3Finalized"()
cdef CResult[c_string] ResolveS3BucketRegion(const c_string& bucket)
```
##########
python/pyarrow/_s3fs.pyx:
##########
@@ -260,6 +271,26 @@ cdef class S3FileSystem(FileSystem):
load_frequency=900, proxy_options=None,
allow_bucket_creation=False, allow_bucket_deletion=False,
retry_strategy: S3RetryStrategy = AwsStandardS3RetryStrategy(max_attempts=3)):
+ ensure_s3_initialized()
+
+ self._initialize_s3(access_key=access_key, secret_key=secret_key, session_token=session_token,
+ anonymous=anonymous, region=region, request_timeout=request_timeout,
+ connect_timeout=connect_timeout, scheme=scheme, endpoint_override=endpoint_override,
+ background_writes=background_writes, default_metadata=default_metadata,
+ role_arn=role_arn, session_name=session_name, external_id=external_id,
+ load_frequency=load_frequency, proxy_options=proxy_options,
+ allow_bucket_creation=allow_bucket_creation, allow_bucket_deletion=allow_bucket_deletion,
+ retry_strategy=retry_strategy)
+
+ def _initialize_s3(self, *, access_key=None, secret_key=None, session_token=None,
+ bint anonymous=False, region=None, request_timeout=None,
+ connect_timeout=None, scheme=None, endpoint_override=None,
+ bint background_writes=True, default_metadata=None,
+ role_arn=None, session_name=None, external_id=None,
+ load_frequency=900, proxy_options=None,
+ allow_bucket_creation=False, allow_bucket_deletion=False,
+ retry_strategy: S3RetryStrategy = AwsStandardS3RetryStrategy(max_attempts=3)):
+
Review Comment:
Do we need this?
It seems that `__init__()` already calls `ensure_s3_initialized()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38364: [Python] Initialize S3 on first use [arrow]
Posted by "pentschev (via GitHub)" <gi...@apache.org>.
pentschev commented on PR #38375:
URL: https://github.com/apache/arrow/pull/38375#issuecomment-1777490148
Thanks @pitrou and @kou for the reviews, truly appreciate the help here! 😄
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38364: [Python] Initialize S3 on first use [arrow]
Posted by "bdice (via GitHub)" <gi...@apache.org>.
bdice commented on PR #38375:
URL: https://github.com/apache/arrow/pull/38375#issuecomment-1777547256
Huge thank you to @kou and @pitrou for your timely review and support! 🙇♂️
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38364: [Python] Initialize S3 on first use [arrow]
Posted by "pentschev (via GitHub)" <gi...@apache.org>.
pentschev commented on code in PR #38375:
URL: https://github.com/apache/arrow/pull/38375#discussion_r1369938151
##########
python/pyarrow/_s3fs.pyx:
##########
@@ -260,6 +271,26 @@ cdef class S3FileSystem(FileSystem):
load_frequency=900, proxy_options=None,
allow_bucket_creation=False, allow_bucket_deletion=False,
retry_strategy: S3RetryStrategy = AwsStandardS3RetryStrategy(max_attempts=3)):
+ ensure_s3_initialized()
+
+ self._initialize_s3(access_key=access_key, secret_key=secret_key, session_token=session_token,
+ anonymous=anonymous, region=region, request_timeout=request_timeout,
+ connect_timeout=connect_timeout, scheme=scheme, endpoint_override=endpoint_override,
+ background_writes=background_writes, default_metadata=default_metadata,
+ role_arn=role_arn, session_name=session_name, external_id=external_id,
+ load_frequency=load_frequency, proxy_options=proxy_options,
+ allow_bucket_creation=allow_bucket_creation, allow_bucket_deletion=allow_bucket_deletion,
+ retry_strategy=retry_strategy)
+
+ def _initialize_s3(self, *, access_key=None, secret_key=None, session_token=None,
+ bint anonymous=False, region=None, request_timeout=None,
+ connect_timeout=None, scheme=None, endpoint_override=None,
+ bint background_writes=True, default_metadata=None,
+ role_arn=None, session_name=None, external_id=None,
+ load_frequency=900, proxy_options=None,
+ allow_bucket_creation=False, allow_bucket_deletion=False,
+ retry_strategy: S3RetryStrategy = AwsStandardS3RetryStrategy(max_attempts=3)):
+
Review Comment:
Indeed they are, thanks for calling my attention to that. Now I see what happened, I initially [tested this in the `12.0.1` tag where CS3Options was not a `std::optional`](https://github.com/apache/arrow/blob/6af660f48472b8b45a5e01b7136b9b040b185eb1/python/pyarrow/_s3fs.pyx#L256) and that was causing the `CS3Options` default constructor to be called before `ensure_s3_initialized()`. I can confirm it works as expected in `main` without that, and fixed it now via https://github.com/apache/arrow/pull/38375/commits/58097e91a305ea05aa7efc24db097a5289b514e4 .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38364: [Python] Initialize S3 on first use [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #38375:
URL: https://github.com/apache/arrow/pull/38375#issuecomment-1778077582
@pentschev No problem! Could you remove the newly added `ensure_s3_initialized()` call ( https://github.com/apache/arrow/pull/38375/files#diff-afa3ea99a387be221ef1f7230aa309b42001aed318cdc6969e700d5eb04d07b2R272-R273 ) in the `S3FileSystem.__init__()` as a follow-up PR? We don't need multiple `ensure_s3_initialized()` calls.
https://github.com/apache/arrow/blob/9e2d4aa2e8743f9302da8823f3c519ab038fc767/python/pyarrow/_s3fs.pyx#L272-L280
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38364: [Python] Initialize S3 on first use [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38375:
URL: https://github.com/apache/arrow/pull/38375#issuecomment-1772614837
:warning: GitHub issue #38364 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38364: [Python] Initialize S3 on first use [arrow]
Posted by "pentschev (via GitHub)" <gi...@apache.org>.
pentschev commented on PR #38375:
URL: https://github.com/apache/arrow/pull/38375#issuecomment-1778594450
Apologies for overlooking that @kou , and good catch! Opened https://github.com/apache/arrow/pull/38451 to address that now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38364: [Python] Initialize S3 on first use [arrow]
Posted by "pentschev (via GitHub)" <gi...@apache.org>.
pentschev commented on PR #38375:
URL: https://github.com/apache/arrow/pull/38375#issuecomment-1781279847
Thanks @jorisvandenbossche for considering that, we indeed saw that pyarrow 14.0 release is in-process, so we opted for [patching conda-forge packages](https://github.com/conda-forge/arrow-cpp-feedstock/pull/1211) which is _probably_ sufficient to overcome the issues we've been experiencing, we'll probably need a couple more days to confirm that but it seems like a promising solution with minimal trouble for Arrow maintainers. 🙂
@bdice @vyasr @jakirkham please feel free to correct me if needed here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38364: [Python] Initialize S3 on first use [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38375:
URL: https://github.com/apache/arrow/pull/38375#issuecomment-1777103939
Revision: 58097e91a305ea05aa7efc24db097a5289b514e4
Submitted crossbow builds: [ursacomputing/crossbow @ actions-06c6a57213](https://github.com/ursacomputing/crossbow/branches/all?query=actions-06c6a57213)
|Task|Status|
|----|------|
|test-conda-python-3.10|[](https://github.com/ursacomputing/crossbow/tree/actions-06c6a57213-github-test-conda-python-3.10)|
|test-conda-python-3.10-cython2|[](https://github.com/ursacomputing/crossbow/actions/runs/6626714591/job/18000196341)|
|test-conda-python-3.10-hdfs-2.9.2|[](https://github.com/ursacomputing/crossbow/actions/runs/6626718302/job/18000207574)|
|test-conda-python-3.10-hdfs-3.2.1|[](https://github.com/ursacomputing/crossbow/actions/runs/6626710907/job/18000185710)|
|test-conda-python-3.10-pandas-latest|[](https://github.com/ursacomputing/crossbow/actions/runs/6626722377/job/18000220344)|
|test-conda-python-3.10-pandas-nightly|[](https://github.com/ursacomputing/crossbow/actions/runs/6626712316/job/18000190356)|
|test-conda-python-3.10-spark-v3.5.0|[](https://github.com/ursacomputing/crossbow/actions/runs/6626718716/job/18000208812)|
|test-conda-python-3.10-substrait|[](https://github.com/ursacomputing/crossbow/actions/runs/6626710557/job/18000184653)|
|test-conda-python-3.11|[](https://github.com/ursacomputing/crossbow/actions/runs/6626715762/job/18000199768)|
|test-conda-python-3.11-dask-latest|[](https://github.com/ursacomputing/crossbow/actions/runs/6626710097/job/18000182764)|
|test-conda-python-3.11-dask-upstream_devel|[](https://github.com/ursacomputing/crossbow/actions/runs/6626710752/job/18000185409)|
|test-conda-python-3.11-hypothesis|[](https://github.com/ursacomputing/crossbow/actions/runs/6626708075/job/18000175977)|
|test-conda-python-3.11-pandas-upstream_devel|[](https://github.com/ursacomputing/crossbow/actions/runs/6626712943/job/18000191930)|
|test-conda-python-3.11-spark-master|[](https://github.com/ursacomputing/crossbow/actions/runs/6626707317/job/18000173899)|
|test-conda-python-3.12|[](https://github.com/ursacomputing/crossbow/actions/runs/6626708569/job/18000177590)|
|test-conda-python-3.8|[](https://github.com/ursacomputing/crossbow/actions/runs/6626711825/job/18000188841)|
|test-conda-python-3.8-pandas-1.0|[](https://github.com/ursacomputing/crossbow/actions/runs/6626709631/job/18000181408)|
|test-conda-python-3.8-spark-v3.5.0|[](https://github.com/ursacomputing/crossbow/actions/runs/6626717135/job/18000203920)|
|test-conda-python-3.9|[](https://github.com/ursacomputing/crossbow/actions/runs/6626711211/job/18000186555)|
|test-conda-python-3.9-pandas-latest|[](https://github.com/ursacomputing/crossbow/actions/runs/6626720181/job/18000213680)|
|test-cuda-python|[](https://github.com/ursacomputing/crossbow/actions/runs/6626721140/job/18000216845)|
|test-debian-11-python-3|[](https://github.com/ursacomputing/crossbow/runs/18000196897)|
|test-fedora-35-python-3|[](https://github.com/ursacomputing/crossbow/runs/18000212722)|
|test-ubuntu-20.04-python-3|[](https://github.com/ursacomputing/crossbow/runs/18000179716)|
|test-ubuntu-22.04-python-3|[](https://github.com/ursacomputing/crossbow/actions/runs/6626717975/job/18000206571)|
|wheel-clean|[](https://github.com/ursacomputing/crossbow/actions/runs/6626714911/job/18000197256)|
|wheel-macos-big-sur-cp310-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626708139/job/18000176448)|
|wheel-macos-big-sur-cp311-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626716558/job/18000202241)|
|wheel-macos-big-sur-cp312-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626713492/job/18000193351)|
|wheel-macos-big-sur-cp38-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626719608/job/18000211708)|
|wheel-macos-big-sur-cp39-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626717595/job/18000205397)|
|wheel-macos-mojave-cp310-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626722187/job/18000219769)|
|wheel-macos-mojave-cp311-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626708836/job/18000178331)|
|wheel-macos-mojave-cp312-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626715536/job/18000199108)|
|wheel-macos-mojave-cp38-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626720047/job/18000213200)|
|wheel-macos-mojave-cp39-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626711607/job/18000188413)|
|wheel-manylinux-2-28-cp310-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626712011/job/18000189746)|
|wheel-manylinux-2-28-cp310-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626713811/job/18000194208)|
|wheel-manylinux-2-28-cp311-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626717838/job/18000206255)|
|wheel-manylinux-2-28-cp311-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626712707/job/18000191524)|
|wheel-manylinux-2-28-cp312-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626717405/job/18000204756)|
|wheel-manylinux-2-28-cp312-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626709314/job/18000180204)|
|wheel-manylinux-2-28-cp38-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626709895/job/18000182216)|
|wheel-manylinux-2-28-cp38-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626716055/job/18000200614)|
|wheel-manylinux-2-28-cp39-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626711449/job/18000187516)|
|wheel-manylinux-2-28-cp39-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626710281/job/18000183470)|
|wheel-manylinux-2014-cp310-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626720562/job/18000215114)|
|wheel-manylinux-2014-cp310-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626714393/job/18000195811)|
|wheel-manylinux-2014-cp311-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626716961/job/18000203330)|
|wheel-manylinux-2014-cp311-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626721924/job/18000219156)|
|wheel-manylinux-2014-cp312-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626709081/job/18000179169)|
|wheel-manylinux-2014-cp312-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626707553/job/18000174727)|
|wheel-manylinux-2014-cp38-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626718934/job/18000209702)|
|wheel-manylinux-2014-cp38-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626716379/job/18000201568)|
|wheel-manylinux-2014-cp39-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626720868/job/18000216024)|
|wheel-manylinux-2014-cp39-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626713145/job/18000192351)|
|wheel-windows-cp310-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626719691/job/18000211994)|
|wheel-windows-cp311-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626709487/job/18000180755)|
|wheel-windows-cp312-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626721438/job/18000217690)|
|wheel-windows-cp38-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626715225/job/18000198202)|
|wheel-windows-cp39-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/6626721625/job/18000218230)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38364: [Python] Initialize S3 on first use [arrow]
Posted by "pentschev (via GitHub)" <gi...@apache.org>.
pentschev commented on code in PR #38375:
URL: https://github.com/apache/arrow/pull/38375#discussion_r1369760262
##########
python/pyarrow/fs.py:
##########
@@ -57,10 +57,6 @@
finalize_s3, initialize_s3, resolve_s3_region)
except ImportError:
_not_imported.append("S3FileSystem")
-else:
- ensure_s3_initialized()
- import atexit
- atexit.register(finalize_s3)
Review Comment:
This seems to work as expected, pushed the changes in https://github.com/apache/arrow/pull/38375/commits/50aa5ee775246026c934ba29c189967d8c083af6. Would you mind taking another look @kou ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38364: [Python] Initialize S3 on first use [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #38375:
URL: https://github.com/apache/arrow/pull/38375#discussion_r1369920039
##########
python/pyarrow/_s3fs.pyx:
##########
@@ -260,6 +271,26 @@ cdef class S3FileSystem(FileSystem):
load_frequency=900, proxy_options=None,
allow_bucket_creation=False, allow_bucket_deletion=False,
retry_strategy: S3RetryStrategy = AwsStandardS3RetryStrategy(max_attempts=3)):
+ ensure_s3_initialized()
+
+ self._initialize_s3(access_key=access_key, secret_key=secret_key, session_token=session_token,
+ anonymous=anonymous, region=region, request_timeout=request_timeout,
+ connect_timeout=connect_timeout, scheme=scheme, endpoint_override=endpoint_override,
+ background_writes=background_writes, default_metadata=default_metadata,
+ role_arn=role_arn, session_name=session_name, external_id=external_id,
+ load_frequency=load_frequency, proxy_options=proxy_options,
+ allow_bucket_creation=allow_bucket_creation, allow_bucket_deletion=allow_bucket_deletion,
+ retry_strategy=retry_strategy)
+
+ def _initialize_s3(self, *, access_key=None, secret_key=None, session_token=None,
+ bint anonymous=False, region=None, request_timeout=None,
+ connect_timeout=None, scheme=None, endpoint_override=None,
+ bint background_writes=True, default_metadata=None,
+ role_arn=None, session_name=None, external_id=None,
+ load_frequency=900, proxy_options=None,
+ allow_bucket_creation=False, allow_bucket_deletion=False,
+ retry_strategy: S3RetryStrategy = AwsStandardS3RetryStrategy(max_attempts=3)):
+
Review Comment:
The given cdef's are `std::optional` and `std::shared_ptr`, so I don't understand the concern.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38364: [Python] Initialize S3 on first use [arrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on PR #38375:
URL: https://github.com/apache/arrow/pull/38375#issuecomment-1781555793
I agree with the above. RAPIDS should be fine with the conda-forge patch and the Arrow 15 bugfix, but it could help other users to get it out sooner.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38364: [Python] Initialize S3 on first use [arrow]
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on PR #38375:
URL: https://github.com/apache/arrow/pull/38375#issuecomment-1781258079
FYI we are in the process of releasing pyarrow 14.0. This PR was just too late for being included, but I did label it as "backport-candidate" in case we need to make a new release candidate or would do a bug-fix release.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org