-
Notifications
You must be signed in to change notification settings - Fork 653
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.
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
FEAT-#7308: Interoperability between query compilers #7376
Conversation
Signed-off-by: arunjose696 <[email protected]>
Signed-off-by: arunjose696 <[email protected]>
Signed-off-by: Igoshev, Iaroslav <[email protected]>
…rialized_dtypes to query compiler layer as in the code in multiple places the methods of private _modin_frame were used
Signed-off-by: Igoshev, Iaroslav <[email protected]>
Co-authored-by: Iaroslav Igoshev <[email protected]> Signed-off-by: arunjose696 <[email protected]>
Co-authored-by: Iaroslav Igoshev <[email protected]>
Signed-off-by: arunjose696 <[email protected]>
…fferent query compilers
Co-authored-by: Iaroslav Igoshev <[email protected]>
fbe9e15
to
6a21aaf
Compare
try: | ||
operation() | ||
# `except` for non callable attributes | ||
except TypeError: |
Check notice
Code scanning / CodeQL
Empty except Note test
modin_series_without_index, _ = create_test_series( | ||
np.arange(col_len), data_frame_mode=data_frame_mode_pair[1] | ||
) | ||
modin_df @ modin_series_without_index |
Check notice
Code scanning / CodeQL
Statement has no effect Note test
6a21aaf
to
cd70589
Compare
cd70589
to
9af55f2
Compare
Signed-off-by: arunjose696 <[email protected]>
9af55f2
to
80ce01c
Compare
assert ( | ||
isinstance(new_query_compiler, type(self._query_compiler)) | ||
or type(new_query_compiler) in self._query_compiler.__class__.__bases__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new_query_compiler can also be an instance of a different query compiler(Nativequerycompiler or PandasQueryCompiler),
Eg during operations like insert in the _create_or_update_from_compiler the constructor gets called directly eg ,
Thus it would be normal for the cases where constructor may return a new query compiler for a case where user changes the query compiler mode between creating data_frame and insert operation
modin/tests/pandas/utils.py
Outdated
if data_frame_mode: | ||
actual_data_frame_mode = NativeDataframeMode().get() | ||
NativeDataframeMode().put(data_frame_mode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arunjose696 I would recommend doing this at a higher level, using fixture modify_config
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would not be possible to use this in the fixture , as the purpose of this is to set two different NativeDataframeMode for 2 different dataframes in the pytest, We dont want to be setting the NativeDataframeMode in the fixture as these dataframes wont be created in the fixture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case let's create create_test_series_in_defined_mode
function in modin/tests/pandas/native_df_mode/utils.py
. Something like:
def create_test_series_in_defined_mode(vals, sort=False, backend=None, df_mode=None, **kwargs):
with context(NativeDataframeMode=df_mode):
return create_test_series(vals, sort=False, backend=None, **kwargs)
And similar for the dataframes.
The main motivation is not to introduce functionality where it is rarely used. Also, given that there is a separate folder with tests, it seems better to try to localize the necessary changes.
Co-authored-by: Iaroslav Igoshev <[email protected]> Co-authored-by: Anatoly Myachev <[email protected]>
modin_df2, pandas_df2 = create_test_dfs(data, backend=backend, df_mode=df_mode2) | ||
md_kwargs, pd_kwargs = {}, {} | ||
|
||
def execute_callable(fn, inplace=False, md_kwargs={}, pd_kwargs={}): |
Check notice
Code scanning / CodeQL
Explicit returns mixed with implicit (fall through) returns Note test
94e8e48
to
af0a7ba
Compare
Signed-off-by: arunjose696 <[email protected]>
af0a7ba
to
8e8ec46
Compare
from modin.core.storage_formats.base.query_compiler import BaseQueryCompiler | ||
from modin.core.storage_formats.pandas.query_compiler_caster import QueryCompilerCaster |
Check notice
Code scanning / CodeQL
Cyclic import Note
modin.core.storage_formats.pandas.query_compiler_caster
|
||
from pandas.core.indexes.frozen import FrozenList | ||
|
||
from modin.core.storage_formats.base.query_compiler import BaseQueryCompiler |
Check notice
Code scanning / CodeQL
Cyclic import Note
modin.core.storage_formats.base.query_compiler
04b7f60
to
03d137c
Compare
Signed-off-by: arunjose696 <[email protected]>
03d137c
to
c1b0942
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arunjose696, please take a look at the failing tests.
Signed-off-by: arunjose696 <[email protected]>
136bf1e
to
f488872
Compare
return staticmethod(apply_argument_cast(obj.__func__)) | ||
|
||
@functools.wraps(obj) | ||
def cast_args(*args: Tuple, **kwargs: Dict) -> Any: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arunjose696 doesn't this function break type hints?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not clear how this would break type hints, as this function is similar to run_and_log
function used in modin logging which would also be wrapping the qc layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not clear how this would break type hints, as this function is similar to run_and_log function used in modin logging which would also be wrapping the qc layer
It should be visible in IDE. Do you see problems with it or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking. | Any
seems redundant, someone will need to sort this out in the future.
What do these changes do?
flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py
black --check modin/ asv_bench/benchmarks scripts/doc_checker.py
git commit -s
docs/development/architecture.rst
is up-to-date