Stricter check for query planning. #107
Conversation
Signed-off-by: Ayush Dattagupta <[email protected]>
Signed-off-by: Ayush Dattagupta <[email protected]>
|
cc: @rjzamora If you could take a look |
| else: | ||
| dask.config.set({"dataframe.query-planning": False}) |
There was a problem hiding this comment.
It doesn't seem like there is any point of this else statement, because the config would already need to be False.
The intended goal is to raise this error whenever |
Right - I believe you have stumbled upon the primary reason we rapids-24.04 was pinned to such an "old" version of dask: There is currently no reliable way (that I know of) to check if the user has already imported That said, it may be possible for us to add a canonical "switch" in |
nemo_curator/__init__.py
Outdated
| @@ -16,11 +16,17 @@ | |||
|
|
|||
| # Disable query planning if possible | |||
| # https://github.com/NVIDIA/NeMo-Curator/issues/73 | |||
| if dask.config.get("dataframe.query-planning") is True: | |||
| QUERY_PLANNING = dask.config.get("dataframe.query-planning") | |||
| if QUERY_PLANNING is True or QUERY_PLANNING is None: | |||
There was a problem hiding this comment.
Thinking a bit more about this, it might make sense to replace the None check for the config with something like "dask_expr" in sys.modules.
We can then followup with a better approach based on the resolution of upstream dask discussions.
Signed-off-by: Ayush Dattagupta <[email protected]>
Co-authored-by: Richard (Rick) Zamora <[email protected]> Signed-off-by: Ayush Dattagupta <[email protected]>
Signed-off-by: Ayush Dattagupta <[email protected]>
ayushdg
requested review from
VibhuJawa and
rjzamora
and removed request for
rjzamora
* Stricter query planning checks with newer versions of dask Signed-off-by: Ayush Dattagupta <[email protected]> * Add checks to tests/__init__ Signed-off-by: Ayush Dattagupta <[email protected]> * Check sys.modules to ensure dask-expr is not enabled Signed-off-by: Ayush Dattagupta <[email protected]> * Search for "dask_expr" in sys modules Co-authored-by: Richard (Rick) Zamora <[email protected]> Signed-off-by: Ayush Dattagupta <[email protected]> * use dask_expr instead of dask-expr Signed-off-by: Ayush Dattagupta <[email protected]> --------- Signed-off-by: Ayush Dattagupta <[email protected]> Co-authored-by: Richard (Rick) Zamora <[email protected]>
Description
Importing newer versions of dask (tested with 2024.5) set
dataframe.query-planningto None instead ofTrueorFalse, but theNonecase defaults to using query planning. This PR extends the check toNoneto raise relevant errors w/ newer versions of dask.Usage
N/A
Checklist