You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@arrow.apache.org by GitBox <gi...@apache.org> on 2022/08/08 22:25:38 UTC
[GitHub] [arrow-adbc] lidavidm opened a new pull request, #57: [Python] Distribute drivers as Python packages
lidavidm opened a new pull request, #57:
URL: https://github.com/apache/arrow-adbc/pull/57
This demonstrates distributing ADBC drivers as Python packages. A Python package for a driver just has a function that exposes the address of the AdbcDriverInitFunc. This is passed to the driver manager, which then initializes everything as usual.
Before the package can be uploaded, this should be converted to use nanoarrow so that it can be truly standalone, and we need build instructions and such. (Ideally after porting to nanoarrow, the Python driver build would just bundle all the source files.)
Also, instead of requiring the user to provide the entrypoint, we should use package entrypoint metadata to auto-discover drivers.
--
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: issues-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #57: [Python] Distribute drivers as Python packages
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #57:
URL: https://github.com/apache/arrow-adbc/pull/57#discussion_r959561742
##########
python/adbc_driver_postgres/pyproject.toml:
##########
@@ -0,0 +1,43 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+[tool.poetry]
+name = "adbc_driver_postgres"
+version = "0.0.0-alpha0"
+description = ""
+authors = ["David Li <li...@gmail.com>"]
Review Comment:
Ah whoops. I fixed both pyproject.toml files.
--
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
[GitHub] [arrow-adbc] lidavidm commented on pull request #57: [Python] Distribute drivers as Python packages
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #57:
URL: https://github.com/apache/arrow-adbc/pull/57#issuecomment-1230302180
Sorry, yes, I had a confusing chain of thought there. It should have been: we don't care about zipimport in any case since it won't work either way.
I'll give the proposed approach a shot, too
--
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
[GitHub] [arrow-adbc] lidavidm commented on pull request #57: [Python] Distribute drivers as Python packages
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #57:
URL: https://github.com/apache/arrow-adbc/pull/57#issuecomment-1231932466
I confirmed auditwheel works with the wheels here, too. I'll defer Windows/MacOS for a future PR since it's a little annoying for me to work with those.
--
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
[GitHub] [arrow-adbc] lidavidm commented on pull request #57: [Python] Distribute drivers as Python packages
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #57:
URL: https://github.com/apache/arrow-adbc/pull/57#issuecomment-1231681973
Updated to not use Cython. It will need work to make it work on MacOS and Windows but the package itself is cleaner 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
[GitHub] [arrow-adbc] lidavidm commented on pull request #57: [Python] Distribute drivers as Python packages
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #57:
URL: https://github.com/apache/arrow-adbc/pull/57#issuecomment-1232940480
Filed #99 to tackle other platforms, instructions, etc.
--
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
[GitHub] [arrow-adbc] kou commented on pull request #57: [Python] Distribute drivers as Python packages
Posted by GitBox <gi...@apache.org>.
kou commented on PR #57:
URL: https://github.com/apache/arrow-adbc/pull/57#issuecomment-1230299583
> but wouldn't that break things like zipimport? (I suppose zipimport just doesn't support extension modules at all.)
I'm not familiar with zipimport but it seems that zipimport doesn't support `.so` built with Cython:
https://docs.python.org/3/library/zipimport.html
> ZIP import of dynamic modules (`.pyd`, `.so`) is disallowed.
But this is not a strong opinion. If you prefer to this approach, I don't opposite it. Thanks for sharing your opinion.
--
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
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #57: [Python] Distribute drivers as Python packages
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #57:
URL: https://github.com/apache/arrow-adbc/pull/57#discussion_r959565062
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -705,18 +724,37 @@ AdbcStatusCode AdbcLoadDriver(const char* driver_name, const char* entrypoint,
#endif // defined(_WIN32)
- auto result = init_func(count, driver, initialized, error);
+ auto result = AdbcLoadDriverFromInitFunc(init_func, count, driver, initialized, error);
+ if (result != ADBC_STATUS_OK) {
#if defined(_WIN32)
- driver->private_manager = new ManagerDriverState{handle, driver->release};
- driver->release = &ReleaseDriver;
+
#endif // defined(_WIN32)
+ }
Review Comment:
I removed this, but I filed #98 since I think we aren't properly calling FreeLibrary on Windows in all cases. (There's some dead code further around L190 that was never wired up).
--
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
[GitHub] [arrow-adbc] lidavidm commented on pull request #57: [Python] Distribute drivers as Python packages
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #57:
URL: https://github.com/apache/arrow-adbc/pull/57#issuecomment-1228933032
So now this works:
```python
import pytest
from adbc_driver_postgres import dbapi
@pytest.fixture
def postgres():
with dbapi.connect(
"postgres://localhost:5432/postgres?user=postgres&password=password"
) as conn:
yield conn
def test_query_trivial(postgres):
with postgres.cursor() as cur:
cur.execute("SELECT 1")
assert cur.fetchone() == (1,)
```
--
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
[GitHub] [arrow-adbc] lidavidm commented on pull request #57: [Python] Distribute drivers as Python packages
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #57:
URL: https://github.com/apache/arrow-adbc/pull/57#issuecomment-1230186290
Hmm, getting rid of the Cython step would be nice, but wouldn't that break things like zipimport? (I suppose zipimport just doesn't support extension modules at all.)
I'd also slightly prefer to take advantage of CPython loading extension modules (and hence being consistent with how it uses dlopen) rather than always doing everything with dlopen/LoadLibrary ourselves, and having to deal with file paths. But I can give it a try and see how it works.
--
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
[GitHub] [arrow-adbc] lidavidm merged pull request #57: [Python] Distribute drivers as Python packages
Posted by GitBox <gi...@apache.org>.
lidavidm merged PR #57:
URL: https://github.com/apache/arrow-adbc/pull/57
--
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
[GitHub] [arrow-adbc] kou commented on pull request #57: [Python] Distribute drivers as Python packages
Posted by GitBox <gi...@apache.org>.
kou commented on PR #57:
URL: https://github.com/apache/arrow-adbc/pull/57#issuecomment-1229799957
Why do we want to avoid the "bundling libadbc_driver_postgres.so to .wheel like pyarrow and use it with AdbcLoadDriver()" approach?
--
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
[GitHub] [arrow-adbc] pitrou commented on pull request #57: [Python] Distribute drivers as Python packages
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #57:
URL: https://github.com/apache/arrow-adbc/pull/57#issuecomment-1230300392
I don't understand the relationship between Cython and zipimport here. Cython produces an extension module in the end...
--
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
[GitHub] [arrow-adbc] kou commented on a diff in pull request #57: [Python] Distribute drivers as Python packages
Posted by GitBox <gi...@apache.org>.
kou commented on code in PR #57:
URL: https://github.com/apache/arrow-adbc/pull/57#discussion_r959046212
##########
python/adbc_driver_postgres/pyproject.toml:
##########
@@ -0,0 +1,43 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+[tool.poetry]
+name = "adbc_driver_postgres"
+version = "0.0.0-alpha0"
+description = ""
+authors = ["David Li <li...@gmail.com>"]
Review Comment:
`"Apache Arrow Developers <de...@arrow.apache.org>"`?
https://github.com/apache/arrow/blob/master/python/setup.py#L766-L767
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -705,18 +724,37 @@ AdbcStatusCode AdbcLoadDriver(const char* driver_name, const char* entrypoint,
#endif // defined(_WIN32)
- auto result = init_func(count, driver, initialized, error);
+ auto result = AdbcLoadDriverFromInitFunc(init_func, count, driver, initialized, error);
+ if (result != ADBC_STATUS_OK) {
#if defined(_WIN32)
- driver->private_manager = new ManagerDriverState{handle, driver->release};
- driver->release = &ReleaseDriver;
+
#endif // defined(_WIN32)
+ }
Review Comment:
It seems that we can remove this `if (result != ADBC_STATUS_OK) { ... }` entirely.
(It seems that we can write `return AdbcLoadDriverFromInitFunc(...)` directly 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