FEAT-#7021: Implement to/from_dask functions #7022

Retribution98 · 2024-03-07T09:38:01Z

What do these changes do?

first commit message and PR title follow format outlined here

NOTE: If you edit the PR title to match this format, you need to add another commit (even if it's empty) or amend your last commit for the CI job that checks the PR title to pick up the new PR title.
passes flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py
passes black --check modin/ asv_bench/benchmarks scripts/doc_checker.py
signed commit with git commit -s
Resolves Implement to/from_dask_dataframe functions #7021
tests added and passing
module layout described at docs/development/architecture.rst is up-to-date

Signed-off-by: Kirill Suvorov <kirill.suvorov@intel.com>

modin/pandas/test/test_io.py

modin/pandas/accessor.py

modin/pandas/test/test_io.py

anmyachev · 2024-03-08T18:10:04Z

modin/core/io/io.py

@@ -140,6 +140,30 @@ def from_ray_dataset(cls, ray_obj):
            "Modin Dataframe can only be converted to a Ray Dataset if Modin uses a Ray engine."
        )

+    @classmethod
+    def from_dask_dataframe(cls, dask_obj):


Let's add a test that catch these exceptions (for example, when ray is using).

No, in the previous peer I implemented the same test for ray, but they demanded that it be removed (you can see this discussion here)

anmyachev · 2024-03-08T18:13:34Z

modin/core/execution/dask/implementations/pandas_on_dask/io/io.py

+import dask.dataframe as dd
+from distributed.client import default_client


Let's use DaskWrapper to interact with Dask API.

Are you sure? As I see it, we use the public API without wrapping it in Wrapper, for both dask and ray.
Also DaskWrapper has annotation: "The class responsible for execution of remote operations.", but creating a Dask Dataframe is not valid for it.

Considering your wishes, I can suggest changing import dask.dataframe as dd to from dask.dataframe import from_delayed. What do you think about it?

Are you sure? As I see it, we use the public API without wrapping it in Wrapper, for both dask and ray.
Also DaskWrapper has annotation: "The class responsible for execution of remote operations.", but creating a Dask Dataframe is not valid for it.

Yes, I'm sure. It's okay to change the description because that's what these classes were designed for. All places where the API is used directly need to be updated to work through a wrapper.

Ok, I see reasons for this, so I just want to clarify. In this case, we just create the same method in DaskWrapper that proxies it to DaskDatafame, like this:

@classmethod def from_delayed(cls, partitions): dd.from_delayed(partitions)

Is correct?

Do I need to create an issue to refactor another public API calls?

Do I need to create an issue to refactor another public API calls?

It would be great.

In this case, we just create the same method in DaskWrapper that proxies it to DaskDatafame, like this:

We discussed it offline, this can also be done separately.

modin/pandas/accessor.py

@@ -31,7 +31,7 @@
 from modin import pandas as pd
 from modin.error_message import ErrorMessage
 from modin.logging import ClassLogger
-from modin.pandas.io import to_ray_dataset
+from modin.pandas.io import to_dask, to_ray_dataset


modin/pandas/test/test_io.py

YarShev · 2024-03-15T13:44:51Z

setup.py

@@ -5,7 +5,7 @@
 with open("README.md", "r", encoding="utf-8") as fh:
    long_description = fh.read()

-dask_deps = ["dask>=2.22.0", "distributed>=2.22.0"]
+dask_deps = ["dask[dataframe]>=2.22.0", "distributed>=2.22.0"]


I think we can leave setup.py without changes to keep the dependencies set minimal for modin-dask and update/align env files with a single dask package. dask[dataframe]/dask-dataframe should be enough for all files. If not, then we should use dask[complete]/dask-complete

No, your suggestion will not help if the library is installed from source or from modin[dask]. This is also important for our CI.
Our env files (environment-dev.yml, requirements-dev.txt) don't need any updates, because they include all required dependencies (dask package for conda and dask[complete] for pip).

In this case, dask[dataframe]` is the best option with the fewest dependencies needed to successfully work with Dask Dataframes.

Our env files (environment-dev.yml, requirements-dev.txt) don't need any updates, because they include all required dependencies (dask package for conda and dask[complete] for pip).

I think we should align these files in terms of dependencies from dask. The package names should match betwen the files. We can do that in this PR or in a separate one.

No, your suggestion will not help if the library is installed from source or from modin[dask]. This is also important for our CI.

Our env files (environment-dev.yml, requirements-dev.txt) don't need any updates, because they include all required dependencies (dask package for conda and dask[complete] for pip).

from_dask and to_dask do not look like the integral methods of Modin. If these methods require an extra package, I think we can consider it as an optional dependency. As to our CI, we can install the required package directly in CI.

modin/pandas/test/test_io.py

docs/ecosystem.rst

modin/core/execution/dask/implementations/pandas_on_dask/io/io.py

YarShev

Please also take a loot at CI, which is failing.

modin/core/execution/dask/implementations/pandas_on_dask/io/io.py

modin/pandas/io.py

modin/pandas/test/test_io.py

Retribution98 · 2024-03-18T16:42:32Z

modin/core/execution/dask/implementations/pandas_on_dask/io/io.py

@@ -171,9 +171,6 @@ def to_dask(cls, modin_obj):
        dask.dataframe.DataFrame or dask.dataframe.Series
            Converted object with type depending on input.
        """
-        import_optional_dependency(
-            "dask-expr", "dask-expr is required to create a Dask DataFrame."
-        )


The default message from dask.dataframe is more user friendly, so I suggest not using this method.

I also think that we should not check the version for dusk_expr. Yes, it has problems in 1.0.1 version, but this is a problem in Dusk, not our compatibility with it. In addition, this library releases new versions very quickly and, in my opinion, users are unlikely to encounter this problem in the future.

FEAT-modin-project#7021: Implement to/from_dask_dataframe functions

2c2a7e7

Signed-off-by: Kirill Suvorov <kirill.suvorov@intel.com>

Retribution98 requested review from devin-petersohn, mvashishtha, RehanSD, YarShev, vnlitvinov, anmyachev, dchigarev and a team as code owners March 7, 2024 09:38

github-advanced-security bot found potential problems Mar 7, 2024

View reviewed changes

modin/pandas/test/test_io.py Fixed Show fixed Hide fixed

modin/pandas/accessor.py Fixed Show fixed Hide fixed

fix after merge

b2db4ff

anmyachev reviewed Mar 8, 2024

View reviewed changes

modin/pandas/test/test_io.py Outdated Show resolved Hide resolved

anmyachev reviewed Mar 8, 2024

View reviewed changes

YarShev reviewed Mar 8, 2024

View reviewed changes

modin/pandas/accessor.py Outdated Show resolved Hide resolved

changes after review

77cea40

github-advanced-security bot found potential problems Mar 13, 2024

View reviewed changes

YarShev changed the title ~~FEAT-#7021: Implement to/from_dask_dataframe functions~~ FEAT-#7021: Implement to/from_dask functions Mar 13, 2024

YarShev reviewed Mar 13, 2024

View reviewed changes

modin/pandas/test/test_io.py Outdated Show resolved Hide resolved

modin/pandas/test/test_io.py Outdated Show resolved Hide resolved

Retribution98 added 3 commits March 14, 2024 11:10

check fix dask import

744f50d

update to from_dask

5cdec20

update test

16b51bb

YarShev reviewed Mar 15, 2024

View reviewed changes

fix inconsistent changes

edc59f7

Retribution98 force-pushed the from_to_dask branch from f554aad to edc59f7 Compare March 18, 2024 10:17

YarShev reviewed Mar 18, 2024

View reviewed changes

modin/core/execution/dask/implementations/pandas_on_dask/io/io.py Outdated Show resolved Hide resolved

modin/pandas/io.py Outdated Show resolved Hide resolved

modin/pandas/test/test_io.py Outdated Show resolved Hide resolved

Retribution98 added 3 commits March 18, 2024 13:37

minor fixes

d6e49f4

remove dask[dataframe] in dependecies

3ced01b

remove import_optional_dependency

7183af8

Retribution98 commented Mar 18, 2024

View reviewed changes

YarShev approved these changes Mar 18, 2024

View reviewed changes

YarShev merged commit 1de52f2 into modin-project:master Mar 18, 2024
53 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEAT-#7021: Implement to/from_dask functions #7022

FEAT-#7021: Implement to/from_dask functions #7022

Retribution98 commented Mar 7, 2024

anmyachev Mar 8, 2024 •

edited

Loading

Retribution98 Mar 11, 2024

anmyachev Mar 8, 2024

Retribution98 Mar 11, 2024

anmyachev Mar 11, 2024

Retribution98 Mar 11, 2024

anmyachev Mar 12, 2024

YarShev Mar 15, 2024

Retribution98 Mar 18, 2024

YarShev Mar 18, 2024

YarShev left a comment

Retribution98 Mar 18, 2024

Retribution98 Mar 18, 2024

Retribution98 Mar 18, 2024

		import dask.dataframe as dd
		from distributed.client import default_client

FEAT-#7021: Implement to/from_dask functions #7022

FEAT-#7021: Implement to/from_dask functions #7022

Conversation

Retribution98 commented Mar 7, 2024

What do these changes do?

anmyachev Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YarShev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmyachev Mar 8, 2024 •

edited

Loading