FEAT-#4245: Define base interface for dataframe exchange protocol #4246

YarShev · 2022-02-22T13:47:42Z

Signed-off-by: Igoshev, Yaroslav yaroslav.igoshev@intel.com

What do these changes do?

commit message follows format outlined here
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 Define base interface for dataframe exchange protocol #4245
tests passing
module layout described at docs/development/architecture.rst is up-to-date
added (Issue Number: PR title (PR Number)) and github username to release notes for next major release

… protocol Signed-off-by: Igoshev, Yaroslav <yaroslav.igoshev@intel.com>

docs/release_notes/release_notes-0.14.0.rst

codecov · 2022-02-22T14:02:24Z

Codecov Report

Merging #4246 (44827f8) into master (47b8a1c) will increase coverage by 3.02%.
The diff coverage is 98.87%.

@@            Coverage Diff             @@
##           master    #4246      +/-   ##
==========================================
+ Coverage   86.80%   89.82%   +3.02%     
==========================================
  Files         201      205       +4     
  Lines       16795    16977     +182     
==========================================
+ Hits        14579    15250     +671     
+ Misses       2216     1727     -489

Impacted Files	Coverage Δ
modin/conftest.py	`92.77% <95.00%> (+9.23%)`	⬆️
...ataframe/base/exchange/dataframe_protocol/utils.py	`98.27% <98.27%> (ø)`
modin/core/dataframe/base/exchange/__init__.py	`100.00% <100.00%> (ø)`
...frame/base/exchange/dataframe_protocol/__init__.py	`100.00% <100.00%> (ø)`
...rame/base/exchange/dataframe_protocol/dataframe.py	`100.00% <100.00%> (ø)`
...core/execution/dispatching/factories/dispatcher.py	`93.33% <100.00%> (+0.18%)`	⬆️
.../core/execution/dispatching/factories/factories.py	`87.70% <100.00%> (+0.20%)`	⬆️
modin/core/io/io.py	`98.41% <100.00%> (+0.02%)`	⬆️
modin/core/storage_formats/base/query_compiler.py	`99.21% <100.00%> (+<0.01%)`	⬆️
...odin/core/storage_formats/pandas/query_compiler.py	`96.18% <100.00%> (+<0.01%)`	⬆️
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 47b8a1c...44827f8. Read the comment docs.

Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>

modin/core/dataframe/base/dataframe/protocol/buffer.py

Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>

Signed-off-by: Igoshev, Yaroslav <yaroslav.igoshev@intel.com>

dchigarev · 2022-02-22T20:50:17Z

modin/pandas/base.py

@@ -556,35 +556,6 @@ def __constructor__(self, *args, **kwargs):
        """
        return type(self)(*args, **kwargs)

-    def __dataframe__(self, nan_as_null: bool = False, allow_copy: bool = True) -> dict:


Why moved? We should be able to export both kinds of objects (Series and DataFrame), so the __dataframe__ method has to be located in the base class.

The spec is only about a DataFrame object, but not a Series.

Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>

dchigarev · 2022-02-23T11:40:00Z

.github/workflows/ci.yml

@@ -256,6 +256,7 @@ jobs:
      - run: python -m pytest -n 2 modin/test/test_partition_api.py
      - run: python -m pytest modin/test/test_utils.py
      - run: python -m pytest asv_bench/test/test_utils.py
+      - run: python -m pytest modin/test/exchange_protocol/base


The most convenient location for the tests to be placed would be near the protocol's code (modin/core/dataframe/base/dataframe/protocol/tests). However, this location creates import conflicts between conftest.py and modin.utils, in particular, this assertion is failing:

modin/modin/conftest.py

Lines 24 to 26 in dae9891

assert (

"modin.utils" not in sys.modules

), "Do not import modin.utils before patching, or tests could fail"

So it was decided to move the tests to a higher level.

That is okay. We actually want to move all tests in one place as part of #3645. However, I would like to generalize the structure a bit. See latest commit.

In order to be ready for adding other exchange protocols.

Agree with @YarShev.

devin-petersohn

Overall structure LGTM!

Signed-off-by: Igoshev, Yaroslav <yaroslav.igoshev@intel.com>

devin-petersohn · 2022-02-23T22:55:25Z

@YarShev this looks much improved. Is it ready to merge?

YarShev · 2022-02-24T05:49:18Z

@devin-petersohn, yes, the structure looks good to me too. I think we can merge this.

anmyachev · 2022-02-24T10:15:32Z

modin/core/storage_formats/base/query_compiler.py

@@ -363,6 +363,61 @@ def to_numpy(self, **kwargs):  # noqa: PR02

    # END To NumPy

+    # Dataframe exchange protocol
+
+    def to_dataframe(self, nan_as_null: bool = False, allow_copy: bool = True) -> dict:


Why not use @abc.abstractmethod decorator? How it's done for to_pandas method. Same for from_dataframe.

In order to be able to instantiate a DataFrame with the underlying QC at this line - https://github.com/modin-project/modin/pull/4246/files#diff-028a94e3db5c73c7ddedc6f7b6efb79e07e1360bb1401531b0a9f5d0ca1f1928R55. Otherwise, you will get the error: Can't instantiate abstract class ... with abstract methods from_dataframe, to_dataframe.

I see, but isn't that the main way Modin defines an interface?

I propose to define the method as abstract in BaseQueryCompiler class, and let the child classes: PandasQueryCompiler and DFAlgQueryCompiler throw NotImplementedError exception.

Yes, BaseQueryCompiler should define the interface to be overridden by child classes. We could either make the methods abstract now or as part of efforts discussed here. I don't have strong preferences on this. However, doing that what you proposed

I propose to define the method as abstract in BaseQueryCompiler class, and let the child classes: PandasQueryCompiler and DFAlgQueryCompiler throw NotImplementedError exception.

You will still be getting the error mentioned above. We should override the methods in TestQC.

Do you think we should make the methods abstract in the Base QC as part of this PR?

If there is no hurry, we can do it here. Otherwise, we need to flag this question and implement it elsewhere. (Yes, it also need to de done in TestQC)

There shouldn't be strong efforts to do that so let's put it here. However, this PR should be merged by end of today.

Yes, it also need to de done in TestQC

That is not quite correct. We should override the methods in TestQC only, PandasQC and OmnisciQC will override the methods as part of new issues.

vnlitvinov · 2022-02-24T12:29:44Z

I would rather we first get data-apis/dataframe-api#74 merged, then vendor that.

YarShev · 2022-02-24T13:42:02Z

To not block development of the implementations for pandas and omnisci storage formats, we can merge this PR after @anmyachev's comment is resolved. Once the API is stabilized in the data-apis repo, we'll just copy it from there and paste it here.

Signed-off-by: Igoshev, Yaroslav <yaroslav.igoshev@intel.com>

YarShev · 2022-02-24T17:41:37Z

TeamCity jobs failed because of our internal fault. TeamCity agents should be added/registered newly. We are working on that. Meanwhile, I think we can omit these jobs and the changes are ready to be merged

modin/core/dataframe/base/exchange/dataframe_protocol/dataframe.py

vnlitvinov · 2022-02-24T17:37:16Z

modin/core/dataframe/base/exchange/dataframe_protocol/utils.py

+    if is_datetime64_dtype(dtype):
+        # Selecting the first char of resolution string:
+        # dtype.str -> '<M8[ns]'
+        resolution = re.findall(r"\[(.*)\]", dtype.str)[0][:1]


isn't there a better way?.. looks weird

cc @dchigarev

We can fix that as part of a separate PR too.

modin/core/storage_formats/pandas/query_compiler.py

modin/test/exchange/dataframe_protocol/base/test_sanity.py

vnlitvinov

Let's Get This Moving!

FEAT-modin-project#4245: Define base interface for dataframe exchange…

b061acf

… protocol Signed-off-by: Igoshev, Yaroslav <yaroslav.igoshev@intel.com>

YarShev requested a review from a team as a code owner February 22, 2022 13:47

YarShev commented Feb 22, 2022

View reviewed changes

docs/release_notes/release_notes-0.14.0.rst Outdated Show resolved Hide resolved

Update docs/release_notes/release_notes-0.14.0.rst

9ef2fa6

dchigarev added 2 commits February 22, 2022 17:24

Add IO for exchange protocol

625059d

Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>

Use enums where possible

741ef0c

Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>

YarShev commented Feb 22, 2022

View reviewed changes

modin/core/dataframe/base/dataframe/protocol/buffer.py Outdated Show resolved Hide resolved

YarShev and others added 6 commits February 22, 2022 18:35

Update modin/core/dataframe/base/dataframe/protocol/buffer.py

265515d

Add common utility function for Arrow C format strings

37cd8a2

Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>

Test exchange protocol utils

83b474f

Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>

Add missing doc-strings

7f966a0

Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>

Combine protocol entities into a single file

826b0a0

Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>

Add sanity testing

2296535

Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>

dchigarev force-pushed the dev/yigoshev-issue4245 branch from b95a8c6 to 2296535 Compare February 22, 2022 17:37

dchigarev added the Ready for review label Feb 22, 2022

dchigarev requested review from devin-petersohn and vnlitvinov February 22, 2022 18:14

Refactor and some fixes

c83c529

Signed-off-by: Igoshev, Yaroslav <yaroslav.igoshev@intel.com>

YarShev force-pushed the dev/yigoshev-issue4245 branch from 65d9dad to c83c529 Compare February 22, 2022 20:42

dchigarev reviewed Feb 22, 2022

View reviewed changes

dchigarev added 2 commits February 23, 2022 14:14

Test IO with no inplace env modifications

c1900e3

Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>

Move protocol tests at higher level to omit import conflicts

83fa0b4

Signed-off-by: Dmitry Chigarev <dmitry.chigarev@intel.com>

dchigarev reviewed Feb 23, 2022

View reviewed changes

devin-petersohn previously approved these changes Feb 23, 2022

View reviewed changes

Generalize the structure

2bb3a6e

Signed-off-by: Igoshev, Yaroslav <yaroslav.igoshev@intel.com>

YarShev dismissed devin-petersohn’s stale review via 2bb3a6e February 23, 2022 16:15

YarShev requested review from dchigarev and devin-petersohn February 23, 2022 16:18

devin-petersohn previously approved these changes Feb 23, 2022

View reviewed changes

anmyachev reviewed Feb 24, 2022

View reviewed changes

YarShev mentioned this pull request Feb 24, 2022

FEAT-#4144: Implement dataframe exchange protocol for pandas storage format #4150

Merged

8 tasks

YarShev dismissed devin-petersohn’s stale review via 6e6a1e8 February 24, 2022 13:52

YarShev requested review from devin-petersohn, anmyachev and dchigarev and removed request for dchigarev February 24, 2022 13:53

YarShev force-pushed the dev/yigoshev-issue4245 branch from 6e6a1e8 to 743ecaf Compare February 24, 2022 14:04

YarShev requested a review from a team as a code owner February 24, 2022 14:04

YarShev force-pushed the dev/yigoshev-issue4245 branch from 743ecaf to b6db18e Compare February 24, 2022 14:06

Apply comment

4fc997b

Signed-off-by: Igoshev, Yaroslav <yaroslav.igoshev@intel.com>

YarShev force-pushed the dev/yigoshev-issue4245 branch from b6db18e to 4fc997b Compare February 24, 2022 14:18

vnlitvinov reviewed Feb 24, 2022

View reviewed changes

YarShev commented Feb 24, 2022

View reviewed changes

modin/test/exchange/dataframe_protocol/base/test_sanity.py Outdated Show resolved Hide resolved

modin/test/exchange/dataframe_protocol/base/test_sanity.py Outdated Show resolved Hide resolved

Apply suggestions from code review

44827f8

YarShev requested a review from vnlitvinov February 24, 2022 18:10

vnlitvinov approved these changes Feb 24, 2022

View reviewed changes

dchigarev approved these changes Feb 24, 2022

View reviewed changes

devin-petersohn approved these changes Feb 24, 2022

View reviewed changes

dchigarev merged commit fc539c3 into modin-project:master Feb 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEAT-#4245: Define base interface for dataframe exchange protocol #4246

FEAT-#4245: Define base interface for dataframe exchange protocol #4246

YarShev commented Feb 22, 2022

codecov bot commented Feb 22, 2022 •

edited

Loading

dchigarev Feb 22, 2022

YarShev Feb 22, 2022

dchigarev Feb 23, 2022

YarShev Feb 23, 2022 •

edited

Loading

YarShev Feb 23, 2022

devin-petersohn Feb 23, 2022

devin-petersohn left a comment

devin-petersohn commented Feb 23, 2022

YarShev commented Feb 24, 2022

anmyachev Feb 24, 2022

YarShev Feb 24, 2022

anmyachev Feb 24, 2022

YarShev Feb 24, 2022

anmyachev Feb 24, 2022

YarShev Feb 24, 2022

YarShev Feb 24, 2022

YarShev Feb 24, 2022

vnlitvinov commented Feb 24, 2022

YarShev commented Feb 24, 2022 •

edited

Loading

YarShev commented Feb 24, 2022

vnlitvinov Feb 24, 2022

YarShev Feb 24, 2022

YarShev Feb 24, 2022

vnlitvinov left a comment

	assert (
	"modin.utils" not in sys.modules
	), "Do not import modin.utils before patching, or tests could fail"

FEAT-#4245: Define base interface for dataframe exchange protocol #4246

FEAT-#4245: Define base interface for dataframe exchange protocol #4246

Conversation

YarShev commented Feb 22, 2022

What do these changes do?

codecov bot commented Feb 22, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YarShev Feb 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devin-petersohn left a comment

Choose a reason for hiding this comment

devin-petersohn commented Feb 23, 2022

YarShev commented Feb 24, 2022

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

vnlitvinov commented Feb 24, 2022

YarShev commented Feb 24, 2022 • edited Loading

YarShev commented Feb 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnlitvinov left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 22, 2022 •

edited

Loading

YarShev Feb 23, 2022 •

edited

Loading

YarShev commented Feb 24, 2022 •

edited

Loading