Closed jorisvandenbossche closed 4 months ago
@jorisvandenbossche can you ping me once you want a review of this? I'll do some reading on dask-expr in the meantime.
I think this should be ready enough for a review
I spent a bit of time looking at the split-out failure, but didn't make much progress. I'll take another look later.
Hi ,thanks for the efforts on supporting dask-expr
.
Any updates on the integration with it ?
Some discussion at https://github.com/dask/dask-expr/issues/1024.
@jorisvandenbossche I can't push to your branch, but this diff fixes the test test_from_dask_dataframe_with_dask_geoseries
. That turned up an error in from_dask_dataframe
with geometry=dask_series
, which is also fixed in that commit (and I added a test for it directly).
PR to your branch at https://github.com/jorisvandenbossche/dask-geopandas/pull/1 that includes that diff and a fix for a couple more issues.
The final xfail with dask-expr is from split_out
. I haven't had a chance to follow up on https://github.com/dask/dask-expr/issues/1024, but it seems like it might be related to shuffling in dask-expr.
@TomAugspurger thanks for the fixes! (and good you mention it here, because apparently you don't watch your own fork by default, so I didn't get any notification from your PR on my repo ..)
@martinfleis unless you have time on the short term, I would suggest we already merge this to have a working main branch with latest dask, and further fixes/improvements can be done in follow-ups
@jorisvandenbossche Go ahead
Tackling https://github.com/geopandas/dask-geopandas/issues/284, for now in a minimal way, i.e. keeping as much as possible the current code based on "legacy" dask and then turn that into an expression.
For example, for sjoin/clip/file IO, we manually build up the graph to create a collection using HighLevelGraph, for now this graph is just turned into an expression with
from_graph
. Long term, we can turn each of those into proper expression classes.I am not yet fully sure how to best organize the code. For now, the new file
expr.py
is essentially duplicating a lot ofcore.py
. I organized the commits such that the first just does this copy, and then you can look at the second commit to see the actual changes that were needed tocore.py
(inexpr.py
) to make it work with dask-expr -> https://github.com/geopandas/dask-geopandas/commit/d28521f30ac0f37b2c0e9c6315eb3921a4896a76The problem is that I can't do this directly, because we want to keep supporting legacy dask for a while as well (our code is actually also still using this for intermediate results)
Some other remarks:
split_out
keyword indissolve
), this is xfailed for nowspatial_partitions
handling is not yet very robust. For now this just takes the same approach as incore.py
to attach this to the collection. But longer term we should attach this to the underlying expression of the collection. The current version "works" (the tests are passing), as long as you only do an operation that needs them directly after an operation that sets them.map_partitions
like before. Longer term, we should turn those into custom expressions (this can then also allow better optimizations in the expression tree)Closes https://github.com/geopandas/dask-geopandas/issues/287