Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 16 commits into from
Feb 25, 2022
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
b061acf
FEAT-#4245: Define base interface for dataframe exchange protocol
YarShev Feb 22, 2022
9ef2fa6
Update docs/release_notes/release_notes-0.14.0.rst
YarShev Feb 22, 2022
625059d
Add IO for exchange protocol
dchigarev Feb 22, 2022
741ef0c
Use enums where possible
dchigarev Feb 22, 2022
265515d
Update modin/core/dataframe/base/dataframe/protocol/buffer.py
YarShev Feb 22, 2022
37cd8a2
Add common utility function for Arrow C format strings
dchigarev Feb 22, 2022
83b474f
Test exchange protocol utils
dchigarev Feb 22, 2022
7f966a0
Add missing doc-strings
dchigarev Feb 22, 2022
826b0a0
Combine protocol entities into a single file
dchigarev Feb 22, 2022
2296535
Add sanity testing
dchigarev Feb 22, 2022
c83c529
Refactor and some fixes
YarShev Feb 22, 2022
c1900e3
Test IO with no inplace env modifications
dchigarev Feb 23, 2022
83fa0b4
Move protocol tests at higher level to omit import conflicts
dchigarev Feb 23, 2022
2bb3a6e
Generalize the structure
YarShev Feb 23, 2022
4fc997b
Apply comment
YarShev Feb 24, 2022
44827f8
Apply suggestions from code review
YarShev Feb 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions modin/core/dataframe/base/dataframe/protocol/test/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,5 @@
# the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF
# ANY KIND, either express or implied. See the License for the specific language
# governing permissions and limitations under the License.

"""Tests for the DataFrame exchange protocol."""
6 changes: 3 additions & 3 deletions modin/core/dataframe/base/dataframe/protocol/test/test_sanity.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,15 @@
# ANY KIND, either express or implied. See the License for the specific language
# governing permissions and limitations under the License.

"""Perform basic sanity checks for the DataFrame exchange protocol."""
"""Basic sanity checks for the DataFrame exchange protocol."""

import pytest
import modin.pandas as pd


def test_sanity():
"""Test that the DataFrame protocol module is valid and could be imported correctly."""
from modin.core.dataframe.base.dataframe.protocol.dataframe import ( # noqa
from modin.core.dataframe.base.dataframe.protocol import ( # noqa
ProtocolDataframe,
)

Expand All @@ -31,7 +31,7 @@ class TestPassed(BaseException):
pass

def dummy_io_method(*args, **kwargs):
"""Dummy method emulating that the code path reached exchange protocol implementation."""
"""Dummy method emulating that the code path reached the exchange protocol implementation."""
raise TestPassed

from modin.core.storage_formats.base.query_compiler import BaseQueryCompiler
Expand Down
2 changes: 1 addition & 1 deletion modin/core/dataframe/base/dataframe/protocol/test/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
# ANY KIND, either express or implied. See the License for the specific language
# governing permissions and limitations under the License.

"""Test common utility functions for the DataFrame exchange protocol."""
"""Tests for common utility functions of the DataFrame exchange protocol."""

import pytest
import pandas
Expand Down
2 changes: 1 addition & 1 deletion modin/core/execution/dispatching/factories/factories.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ def _from_non_pandas(cls, *args, **kwargs):
@classmethod
@doc(
_doc_io_method_template,
source="a DataFrame object supporting exchange protocol (__dataframe__())",
source="a DataFrame object supporting exchange protocol `__dataframe__()`",
params=_doc_io_method_all_params,
method="utils.from_dataframe",
)
Expand Down
4 changes: 2 additions & 2 deletions modin/core/io/io.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,12 +105,12 @@ def from_arrow(cls, at):
@classmethod
def from_dataframe(cls, df):
"""
Create a Modin `query_compiler` from a DataFrame object supporting exchange protocol (__dataframe__()).
Create a Modin QueryCompiler from a DataFrame supporting the DataFrame exchange protocol `__dataframe__()`.

Parameters
----------
df : DataFrame
The DataFrame object supporting dataframe exchange protocol.
The DataFrame object supporting the DataFrame exchange protocol.

Returns
-------
Expand Down
14 changes: 7 additions & 7 deletions modin/core/storage_formats/base/query_compiler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ def to_numpy(self, **kwargs): # noqa: PR02

# END To NumPy

# Exchange protocol
# Dataframe exchange protocol

def to_dataframe(self, nan_as_null: bool = False, allow_copy: bool = True) -> dict:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

"""
Expand All @@ -388,21 +388,21 @@ def to_dataframe(self, nan_as_null: bool = False, allow_copy: bool = True) -> di
Returns
-------
dict
A dictionary object following the dataframe protocol specification.
A dictionary object following the DataFrame protocol specification.
"""
raise NotImplementedError(
"The selected execution does not implement DataFrame exchange protocol."
"The selected execution does not implement the DataFrame exchange protocol."
)

@classmethod
def from_dataframe(cls, df, data_cls):
"""
Build QueryCompiler from a DataFrame object supporting dataframe exchange protocol (__dataframe__()).
Build QueryCompiler from a DataFrame object supporting the dataframe exchange protocol `__dataframe__()`.

Parameters
----------
df : DataFrame
The DataFrame object supporting dataframe exchange protocol.
The DataFrame object supporting the dataframe exchange protocol.
data_cls : type
:py:class:`~modin.core.dataframe.pandas.dataframe.dataframe.PandasDataframe` class
(or its descendant) to convert to.
Expand All @@ -413,10 +413,10 @@ def from_dataframe(cls, df, data_cls):
QueryCompiler containing data from the DataFrame.
"""
raise NotImplementedError(
"The selected execution does not implement import via dataframe exchange protocol."
"The selected execution does not implement import via the DataFrame exchange protocol."
)

# END Exchange protocol
# END Dataframe exchange protocol

# Abstract inter-data operations (e.g. add, sub)
# These operations require two DataFrames and will change the shape of the
Expand Down
29 changes: 0 additions & 29 deletions modin/pandas/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"""
Get a DataFrame exchange protocol object representing data of the Modin DataFrame.

See more about the protocol in https://data-apis.org/dataframe-protocol/latest/index.html.

Parameters
----------
nan_as_null : bool, default: False
A keyword intended for the consumer to tell the producer
to overwrite null values in the data with ``NaN`` (or ``NaT``).
This currently has no effect; once support for nullable extension
dtypes is added, this value should be propagated to columns.
allow_copy : bool, default: True
A keyword that defines whether or not the library is allowed
to make a copy of the data. For example, copying data would be necessary
if a library supports strided buffers, given that this protocol
specifies contiguous buffers. Currently, if the flag is set to ``False``
and a copy is needed, a ``RuntimeError`` will be raised.

Returns
-------
dict
A dictionary object following the dataframe protocol specification.
"""
return self._query_compiler.to_dataframe(
nan_as_null=nan_as_null, allow_copy=allow_copy
)

def abs(self):
self._validate_dtypes(numeric_only=True)
return self.__constructor__(query_compiler=self._query_compiler.abs())
Expand Down
29 changes: 29 additions & 0 deletions modin/pandas/dataframe.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2623,6 +2623,35 @@ def __delitem__(self, key):
__rmod__ = rmod
__rdiv__ = rdiv

def __dataframe__(self, nan_as_null: bool = False, allow_copy: bool = True) -> dict:
"""
Get a Modin DataFrame that implements the dataframe exchange protocol.

See more about the protocol in https://data-apis.org/dataframe-protocol/latest/index.html.

Parameters
----------
nan_as_null : bool, default: False
A keyword intended for the consumer to tell the producer
to overwrite null values in the data with ``NaN`` (or ``NaT``).
This currently has no effect; once support for nullable extension
dtypes is added, this value should be propagated to columns.
allow_copy : bool, default: True
A keyword that defines whether or not the library is allowed
to make a copy of the data. For example, copying data would be necessary
if a library supports strided buffers, given that this protocol
specifies contiguous buffers. Currently, if the flag is set to ``False``
and a copy is needed, a ``RuntimeError`` will be raised.

Returns
-------
dict
A dictionary object following the dataframe protocol specification.
"""
return self._query_compiler.to_dataframe(
nan_as_null=nan_as_null, allow_copy=allow_copy
)

@property
def attrs(self): # noqa: D200
"""
Expand Down
4 changes: 2 additions & 2 deletions modin/pandas/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,14 +89,14 @@ def from_arrow(at):

def from_dataframe(df):
"""
Convert a DataFrame implementing dataframe exchange protocol to a Modin DataFrame.
Convert a DataFrame implementing the dataframe exchange protocol to a Modin DataFrame.

See more about the protocol in https://data-apis.org/dataframe-protocol/latest/index.html.

Parameters
----------
df : DataFrame
The DataFrame object supporting dataframe exchange protocol.
The DataFrame object supporting the dataframe exchange protocol.

Returns
-------
Expand Down