You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/04/26 14:46:51 UTC
[GitHub] [arrow] amol- opened a new pull request #10162: ARROW-12506: [Python] Improve modularity of pyarrow codebase: _ipc module
amol- opened a new pull request #10162:
URL: https://github.com/apache/arrow/pull/10162
Third batch of changes related to making pyarrow build more modular. ipc is no longer included in pyarrow.lib but has been separated to its own `_ipc` module.
This PR is based on #10159
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10162: ARROW-12506: [Python] Improve modularity of pyarrow codebase: _ipc module
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10162:
URL: https://github.com/apache/arrow/pull/10162#issuecomment-826903864
https://issues.apache.org/jira/browse/ARROW-12506
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10162: ARROW-12506: [Python] Improve modularity of pyarrow codebase: _ipc module
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10162:
URL: https://github.com/apache/arrow/pull/10162#issuecomment-887327342
I would support merging this (once it's rebased and conflicts are fixed). We can have a discussion about public Cython APIs later.
--
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] amol- commented on pull request #10162: ARROW-12506: [Python] Improve modularity of pyarrow codebase: _ipc module
Posted by GitBox <gi...@apache.org>.
amol- commented on pull request #10162:
URL: https://github.com/apache/arrow/pull/10162#issuecomment-830000884
Passed CI and ready for review
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorisvandenbossche commented on pull request #10162: ARROW-12506: [Python] Improve modularity of pyarrow codebase: _ipc module
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on pull request #10162:
URL: https://github.com/apache/arrow/pull/10162#issuecomment-892015848
I think it would be better to first discuss more what we think the "public Cython API" story should be. Because there currently are packages that use the cython APIs, and this PR might break those packages, without a clear message what they should do instead.
(in practice they might mostly be using things that are not from the IPC module, though, so this specific PR might not cause much breakage).
Previous time we discussed this I looked for some example packages that interfaces with pyarrow on the cython level:
* BlazingSQL (https://github.com/BlazingDB/blazingsql/blob/94aa7afd1feaba63cf9853eec3824b95b47b46bd/engine/bsql_engine/io/io.pyx, although not directly clear what they are using since they are doing a `*` import)
* Rapids / cudf (https://github.com/rapidsai/cudf/blob/main/python/cudf/cudf/_lib/gpuarrow.pyx)
* Cylon (https://github.com/cylondata/cylon/search?q=%22pyarrow.lib%22)
* Fletcher (https://github.com/abs-tudelft/fletcher/blob/dbe3b3aa20fce72f69341aaa79230b60d79b4efa/runtime/python/pyfletcher/includes/libfletcher.pxd)))
* Omnisci (https://github.com/omnisci/omniscidb/blob/a667adc9c879edcf8134e52ea5fcf4be234052b9/Embedded/Python/dbe.pyx)
* katana (https://github.com/KatanaGraph/katana/search?q=%22pyarrow.lib%22)
* mongo-arrow (https://github.com/mongodb-labs/mongo-arrow/blob/main/bindings/python/pymongoarrow/builders.pyi)
--
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] jorisvandenbossche commented on pull request #10162: ARROW-12506: [Python] Improve modularity of pyarrow codebase: _ipc module
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on pull request #10162:
URL: https://github.com/apache/arrow/pull/10162#issuecomment-831842220
> In general I feel that `lib` is something internal
Fully agreed on the python side, but I am not sure for cython:
> I don't think we need to support Cython imports, except for the functions described here.
That is not what happens in practice. Even our own example at https://arrow.apache.org/docs/python/extending.html#example, includes a `from pyarrow.lib cimport *` (from which it uses `CArray` in this case, which is already not explicitly listed in the Cython API doc).
Of course something like `CArray` is part of the signature of the functions, so that can clearly be included as well.
But what about `CNullArray`? (eg used in our own [test example](https://github.com/apache/arrow/blob/master/python/pyarrow/tests/pyarrow_cython_example.pyx))
And what about the non-C versions, like `Array` ?
We could certainly restrict what we see as public cython API, but then we should start with more explicitly documenting it, if it's not "whatever is exposed in `pyarrow/lib.pxd` / by `from pyarrow.lib cimport ...`"
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] amol- commented on pull request #10162: ARROW-12506: [Python] Improve modularity of pyarrow codebase: _ipc module
Posted by GitBox <gi...@apache.org>.
amol- commented on pull request #10162:
URL: https://github.com/apache/arrow/pull/10162#issuecomment-831834231
> Can you give some details on what's the backwards-compatibility story is here?
>
> I think there are two aspects: python-level and cython-level.
> In cython, for people doing `from pyarrow.lib cimport RecordBatchReader`, this will stop working?
> In python, if you accessed the IPC APIs from `pyarrow.lib`, this will also stop working (although I think this is less of a concern, as it was clearly documented otherwise)
I retained compatibility with `pyarrow.ipc` module, so doing `from pyarrow.ipc import RecordBatchReader` will continue to work. While `from pyarrow.lib import RecordBatchReader` as you mentioned will stop working.
I personally thing that classes should have one and only one clear owning module, and that seemed to be `ipc` in this case. In general I feel that `lib` is something internal (apart for the explicitly documented things that @pitrou mentioned) and it smells fragile on long term to spread the usage of internals from third parties.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] amol- commented on pull request #10162: ARROW-12506: [Python] Improve modularity of pyarrow codebase: _ipc module
Posted by GitBox <gi...@apache.org>.
amol- commented on pull request #10162:
URL: https://github.com/apache/arrow/pull/10162#issuecomment-831918769
> We could certainly restrict what we see as public cython API, but then we should start with more explicitly documenting it, if it's not "whatever is exposed in `pyarrow/lib.pxd` / by `from pyarrow.lib cimport ...`"
I feel that would be a good idea. I feel that pyarrow would benefit from a more clear separation between internals and public facing API. Given that Python doesn't usually foster that kind of separation very well it becomes even more important for frameworks and libraries to make clear what's the contract with users.
Otherwise the risk is to make user experience terrible and get the project perceived as fragile (as every time you change something you break someone else code) while also having an hard life every time you want to change/refactor something as you never know if you are going to break someone else code.
Imho a clear definition of the public facing API is as important as automated tests for a library/framework.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] amol- commented on pull request #10162: ARROW-12506: [Python] Improve modularity of pyarrow codebase: _ipc module
Posted by GitBox <gi...@apache.org>.
amol- commented on pull request #10162:
URL: https://github.com/apache/arrow/pull/10162#issuecomment-878276520
Rebased against master
--
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] jorisvandenbossche commented on pull request #10162: ARROW-12506: [Python] Improve modularity of pyarrow codebase: _ipc module
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on pull request #10162:
URL: https://github.com/apache/arrow/pull/10162#issuecomment-831829432
Can you give some details on what's the backwards-compatibility story is here?
I think there are two aspects: python-level and cython-level.
In cython, for people doing `from pyarrow.lib cimport RecordBatchReader`, this will stop working?
In python, if you accessed the IPC APIs from `pyarrow.lib`, this will also stop working (although I think this is less of a concern, as it was clearly documented otherwise)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] amol- edited a comment on pull request #10162: ARROW-12506: [Python] Improve modularity of pyarrow codebase: _ipc module
Posted by GitBox <gi...@apache.org>.
amol- edited a comment on pull request #10162:
URL: https://github.com/apache/arrow/pull/10162#issuecomment-831834231
> Can you give some details on what's the backwards-compatibility story is here?
>
> I think there are two aspects: python-level and cython-level.
> In cython, for people doing `from pyarrow.lib cimport RecordBatchReader`, this will stop working?
> In python, if you accessed the IPC APIs from `pyarrow.lib`, this will also stop working (although I think this is less of a concern, as it was clearly documented otherwise)
I retained compatibility with `pyarrow.ipc` module, so doing `from pyarrow.ipc import RecordBatchReader` will continue to work. While `from pyarrow.lib import RecordBatchReader` as you mentioned will stop working.
I personally think that classes should have one and only one clear owning module, and that seemed to be `ipc` in this case. In general I feel that `lib` is something internal (apart for the explicitly documented things that @pitrou mentioned) and it smells fragile on long term to spread the usage of internals from third parties.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10162: ARROW-12506: [Python] Improve modularity of pyarrow codebase: _ipc module
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10162:
URL: https://github.com/apache/arrow/pull/10162#issuecomment-831831200
I don't think we need to support Cython imports, except for the functions described [here](https://arrow.apache.org/docs/python/extending.html#cython-api).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org