FEAT-#6767: Provide the ability to use experimental functionality when experimental mode is not enabled globally via an environment variable #6764

anmyachev · 2023-11-23T12:16:05Z

What do these changes do?

Main changes:

~~New enable_exp_mode context manager~~
New attribute for accessing experimental functionality: modin = CachedAccessor("modin", ExperimentalFunctions)
- This is done in order to avoid adding new methods to the dataframe when importing module modin.experimental.pandas. ~~When switching between two modes (Experimental and non experimental), it would be quite difficult to remove/add them again.~~
- It is not advisable to add experimental functions to the main namespace, so as not to mix them with pandas functions and confuse the users.
~~As a side effect, the factory is now initialized more than once when using enable_exp_mode context manager.~~

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 Provide the ability to use experimental functionality when experimental mode is not enabled globally via an environment variable. #6767
tests added and passing
module layout described at docs/development/architecture.rst is up-to-date

modin/experimental/pandas/__init__.py

modin/utils.py

…ext manager Signed-off-by: Anatoly Myachev <[email protected]>

anmyachev · 2023-11-23T23:39:24Z

modin/utils.py

+        FactoryDispatcher._update_factory_if_already_initialized()
+        yield
+    finally:
+        IsExperimental.put(old_value or False)


old_value can be None

modin/test/storage_formats/pandas/test_internals.py

dchigarev · 2023-11-24T10:28:28Z

modin/pandas/accessor.py

+    """
+
+    def __init__(self, data):
+        if IsExperimental.get() is not True:


Can we get rid of this requirement? I mean, if a user already gone that far by accessing an underscore df._exp field, then he probably understands what he's doing and his intention to use experimental API is certain enough.

we could have added a decorator for each of the methods in ExperimentalFunctions that would enable/disable experimental mode in the fly

@dchigarev done, ready for review

Signed-off-by: Anatoly Myachev <[email protected]>

modin/pandas/accessor.py

dchigarev · 2023-11-24T14:53:25Z

modin/utils.py

+    old_value = IsExperimental.get()
+    try:
+        IsExperimental.put(True)
+        FactoryDispatcher._update_factory_if_already_initialized()


can we hide the factory updating to the IsExperimental.put() method? We can then make this context function generic, so it could work with other config variables that we tend to change often (for example, ExperimentalGroupbyImpl, ExperimentalNumpyAPI, AsyncReading, ...):

with modin.utils.set_config_value(IsExperimental, value=True): ...

An interesting idea, it seems possible, but let's check it separately.

dchigarev · 2023-11-24T15:12:58Z

modin/experimental/pandas/__init__.py

-setattr(DataFrame, "to_pickle_distributed", to_pickle_distributed)  # noqa: F405
-


why? it seems like it breaks the old use-case:

from modin.config import IsExperimental IsExperimental.put(True) import modin.experimental.pandas as pd pd.DataFrame(...).to_pickle_distributed() # fails

I would expect the new behavior via ._exp to be adding a new way of invoking experimental method, but not replacing the previous one

Yes, it breaks. Initially, this was done without taking into account the case when the experimental mode should work in conjunction with the regular one. Updating attributes during import makes context switching very difficult.

Since this is experimental code, I thought that such a breaking change would not be a big problem.

Updating attributes during import makes context switching very difficult

why? shouldn't enable_exp_mode context do nothing if the experimental mode is already enabled?

enable_exp_mode context do nothing if the experimental mode is already enabled?

Yes, but what if import modin.experimental.pandas is made in this context? The expected behavior would be that when exiting the context, no side effects remain.

Well, it still should be fine then. If I understand correctly, after we leave the context, the experimental methods of a DataFrame will still be accessible but not functioning, which seems like a pretty logical behavior

with enable_exp_mode(): import modin.experimental.pandas as pd pd.DataFrame([]).to_pickle_distributed() # works pd.DataFrame([]).to_pickle_distributed() # raises: not in experimental mode

I brought back this function, but added a warning about future removal and an alternative to use. I hope this suits you.

dchigarev · 2023-11-24T15:19:33Z

modin/pandas/dataframe.py

+    # Namespace for experimental functions
+    _exp = CachedAccessor("_exp", ExperimentalFunctions)


adding a private accessor to be used by users so they can access experimental functionality is fine with me, however, would like to hear opinions from others @Garra1980 @YarShev

Signed-off-by: Anatoly Myachev <[email protected]>

YarShev · 2023-11-27T18:52:16Z

Maybe we should initialize both a primary factory and a respective expetimental factory at once and expose experimental pandas API that is slightly different to the original one? Doing so we could avoid an extra accessor _exp and a new context manager enable_exp_mode, which seems to me not user-friendly a bit.

anmyachev · 2023-11-28T00:52:28Z

and expose experimental pandas API that is slightly different to the original one

Do you mean to provide access to experimental functions in the same namespace as regular functions, but which will only work when experimental mode is enabled?

Maybe we should initialize both a primary factory and a respective expetimental factory at once

Moreover, we could simply combine them into one.

YarShev · 2023-11-28T09:37:23Z

Do you mean to provide access to experimental functions in the same namespace as regular functions, but which will only work when experimental mode is enabled?

Through modin.experimental.pandas and the experimental API shouldn't be depending on MODIN_EXPERIMENTAL.

Moreover, we could simply combine them into one.

We could probably combine some of them into one, and, I think since we have the executions such as ExpetimentalPyarrowOnRay and ExperimentalHdkOnNative, we should provide them when MODIN_EXPERIMENTAL is enabled, i.e. as is now.

anmyachev · 2023-11-28T22:03:48Z

Through modin.experimental.pandas and the experimental API shouldn't be depending on MODIN_EXPERIMENTAL.

@YarShev I don’t really understand how in this case we will be able to provide access to experimental dataframe methods. Could you provide some more details?

Signed-off-by: Anatoly Myachev <[email protected]>

anmyachev · 2023-12-05T19:56:08Z

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

+    read_sql_distributed = __make_read(
+        ExperimentalSQLDispatcher, build_args={**build_args, "base_read": read_sql}
+    )


The experimental implementation of reading sql on the master is called a non-experimental implementation of the same function. Due to the fact that classes are built from several other classes at runtime, I have not found a more beautiful way to do the same thing, given the new class architecture (in the PR).

Signed-off-by: Anatoly Myachev <[email protected]>

anmyachev · 2023-12-05T23:16:38Z

modin/experimental/pandas/io.py

@@ -115,7 +115,7 @@ def read_sql(

    assert IsExperimental.get(), "This only works in experimental mode"

-    result = FactoryDispatcher.read_sql(**kwargs)
+    result = FactoryDispatcher.read_sql_distributed(**kwargs)


The new name is for internal use only.

modin/core/execution/dispatching/factories/test/test_dispatcher.py

modin/experimental/core/io/sql/sql_dispatcher.py

docs/usage_guide/advanced_usage/index.rst

modin/core/execution/dispatching/factories/dispatcher.py

modin/core/execution/dispatching/factories/factories.py

modin/experimental/pandas/__init__.py

modin/experimental/pandas/io.py

modin/experimental/pandas/test/test_io_exp.py

modin/pandas/accessor.py

modin/pandas/dataframe.py

modin/pandas/test/test_api.py

Signed-off-by: Anatoly Myachev <[email protected]>

…to-pickle-new

modin/core/execution/dispatching/factories/dispatcher.py

YarShev · 2023-12-07T09:00:06Z

modin/core/execution/dispatching/factories/factories.py

+    )
+    def _read_sql_distributed(cls, **kwargs):
+        current_execution = get_current_execution()
+        if current_execution not in supported_executions:


Should we move this check into existing is_distributed method?

This requires adding the following attribute to all IO classes:

read_sql_distributed = __make_read( ExperimentalSQLDispatcher, build_args={**build_args, "base_read": read_sql} )

Looks like a complication.

YarShev · 2023-12-07T09:03:50Z

Left a few comments + there are some not addressed ones. Other than that, LGTM.

docs/usage_guide/advanced_usage/index.rst

Signed-off-by: Anatoly Myachev <[email protected]>

anmyachev · 2023-12-07T15:11:47Z

docs/flow/modin/experimental/pandas.rst

@@ -13,4 +13,4 @@ Experimental API Reference
 .. autofunction:: read_csv_glob
 .. autofunction:: read_custom_text
 .. autofunction:: read_pickle_distributed
-.. automethod:: modin.experimental.pandas.DataFrame.to_pickle_distributed
+.. automethod:: modin.pandas.DataFrame.modin::to_pickle_distributed


As a hint to autodoc extension, you can put a :: separator in between module name and object name to let autodoc know the correct module name if it is ambiguous. from https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#directive-automethod

YarShev · 2023-12-07T17:30:24Z

@dchigarev, @Garra1980, any other comments?

anmyachev changed the title ~~FEAT-#0000: make pickling methods available when experimental mode is activated in runtime~~ FEAT-#6767: Implement 'modin.utils.enable_exp_mode' context manager Nov 23, 2023

anmyachev force-pushed the to-pickle-new branch from cd64066 to f5ba5a7 Compare November 23, 2023 17:16

anmyachev commented Nov 23, 2023

View reviewed changes

modin/experimental/pandas/__init__.py Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Nov 23, 2023

View reviewed changes

modin/utils.py Fixed Show fixed Hide fixed

anmyachev force-pushed the to-pickle-new branch 2 times, most recently from 6e141e8 to 31dfdf1 Compare November 23, 2023 22:24

anmyachev changed the title ~~FEAT-#6767: Implement 'modin.utils.enable_exp_mode' context manager~~ FEAT-#6767: Implement modin.utils.enable_exp_mode context manager Nov 23, 2023

FEAT-modin-project#6767: Implement 'modin.utils.enable_exp_mode' cont…

e1caebb

…ext manager Signed-off-by: Anatoly Myachev <[email protected]>

anmyachev force-pushed the to-pickle-new branch from 31dfdf1 to e1caebb Compare November 23, 2023 23:38

anmyachev commented Nov 23, 2023

View reviewed changes

github-advanced-security bot found potential problems Nov 23, 2023

View reviewed changes

modin/test/storage_formats/pandas/test_internals.py Fixed Show fixed Hide fixed

anmyachev marked this pull request as ready for review November 23, 2023 23:52

anmyachev requested review from devin-petersohn, mvashishtha, RehanSD, YarShev, vnlitvinov, dchigarev and a team as code owners November 23, 2023 23:52

dchigarev reviewed Nov 24, 2023

View reviewed changes

address review comments

937e072

Signed-off-by: Anatoly Myachev <[email protected]>

github-advanced-security bot found potential problems Nov 24, 2023

View reviewed changes

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

dchigarev reviewed Nov 24, 2023

View reviewed changes

address review comments

db82052

Signed-off-by: Anatoly Myachev <[email protected]>

anmyachev added 3 commits December 5, 2023 15:53

revert experimental sql dispatcher changes

42dea89

Signed-off-by: Anatoly Myachev <[email protected]>

fixes

41e4cc9

Signed-off-by: Anatoly Myachev <[email protected]>

fixes

bee5206

Signed-off-by: Anatoly Myachev <[email protected]>

anmyachev force-pushed the to-pickle-new branch from 33818d3 to bee5206 Compare December 5, 2023 17:48

fix hdk

efc1492

Signed-off-by: Anatoly Myachev <[email protected]>

anmyachev commented Dec 5, 2023

View reviewed changes

address review comments

dead18a

Signed-off-by: Anatoly Myachev <[email protected]>

anmyachev force-pushed the to-pickle-new branch from 2fc9113 to dead18a Compare December 5, 2023 20:01

fix internal test

b2c1943

Signed-off-by: Anatoly Myachev <[email protected]>

anmyachev changed the title ~~FEAT-#6767: Implement modin.utils.enable_exp_mode context manager~~ FEAT-#6767: Provide the ability to use experimental functionality when experimental mode is not enabled globally via an environment variable Dec 5, 2023

anmyachev added 2 commits December 6, 2023 00:12

update docs

1be0fb3

Signed-off-by: Anatoly Myachev <[email protected]>

change namespace: '_exp' -> 'modin'

dd8365a

Signed-off-by: Anatoly Myachev <[email protected]>

anmyachev commented Dec 5, 2023

View reviewed changes

modin/core/execution/dispatching/factories/test/test_dispatcher.py Show resolved Hide resolved

anmyachev commented Dec 5, 2023

View reviewed changes

modin/experimental/core/io/sql/sql_dispatcher.py Show resolved Hide resolved

YarShev reviewed Dec 6, 2023

View reviewed changes

anmyachev added 2 commits December 7, 2023 00:53

address review comments

a57d04f

Signed-off-by: Anatoly Myachev <[email protected]>

Merge branch 'master' of https://github.com/modin-project/modin into …

e05221b

…to-pickle-new

YarShev reviewed Dec 7, 2023

View reviewed changes

docs/usage_guide/advanced_usage/index.rst Outdated Show resolved Hide resolved

address review comments

82f50cc

Signed-off-by: Anatoly Myachev <[email protected]>

anmyachev commented Dec 7, 2023

View reviewed changes

YarShev approved these changes Dec 7, 2023

View reviewed changes

dchigarev mentioned this pull request Dec 8, 2023

Remove 'experimental' from the name of ExperimentalNumpyAPI variable #6814

Closed

dchigarev approved these changes Dec 8, 2023

View reviewed changes

YarShev merged commit c3a4f78 into modin-project:master Dec 8, 2023
53 checks passed

anmyachev deleted the to-pickle-new branch December 8, 2023 17:34

YarShev mentioned this pull request Dec 15, 2023

Don't implicitly enable experimental mode #6818

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEAT-#6767: Provide the ability to use experimental functionality when experimental mode is not enabled globally via an environment variable #6764

FEAT-#6767: Provide the ability to use experimental functionality when experimental mode is not enabled globally via an environment variable #6764

anmyachev commented Nov 23, 2023 •

edited

Loading

anmyachev Nov 23, 2023

dchigarev Nov 24, 2023

dchigarev Nov 24, 2023

anmyachev Nov 24, 2023

dchigarev Nov 24, 2023 •

edited

Loading

anmyachev Nov 24, 2023

dchigarev Nov 24, 2023 •

edited

Loading

dchigarev Nov 24, 2023

anmyachev Nov 24, 2023

dchigarev Nov 24, 2023

anmyachev Nov 24, 2023

dchigarev Nov 27, 2023

anmyachev Nov 27, 2023

dchigarev Nov 24, 2023

YarShev commented Nov 27, 2023

anmyachev commented Nov 28, 2023

YarShev commented Nov 28, 2023 •

edited

Loading

anmyachev commented Nov 28, 2023

anmyachev Dec 5, 2023

anmyachev Dec 5, 2023

YarShev Dec 7, 2023

anmyachev Dec 7, 2023

YarShev commented Dec 7, 2023

anmyachev Dec 7, 2023

YarShev commented Dec 7, 2023

		setattr(DataFrame, "to_pickle_distributed", to_pickle_distributed) # noqa: F405

		# Namespace for experimental functions
		_exp = CachedAccessor("_exp", ExperimentalFunctions)

FEAT-#6767: Provide the ability to use experimental functionality when experimental mode is not enabled globally via an environment variable #6764

FEAT-#6767: Provide the ability to use experimental functionality when experimental mode is not enabled globally via an environment variable #6764

Conversation

anmyachev commented Nov 23, 2023 • edited Loading

What do these changes do?

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

dchigarev Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dchigarev Nov 24, 2023 • 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

YarShev commented Nov 27, 2023

anmyachev commented Nov 28, 2023

YarShev commented Nov 28, 2023 • edited Loading

anmyachev commented Nov 28, 2023

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 commented Dec 7, 2023

Choose a reason for hiding this comment

YarShev commented Dec 7, 2023

anmyachev commented Nov 23, 2023 •

edited

Loading

dchigarev Nov 24, 2023 •

edited

Loading

dchigarev Nov 24, 2023 •

edited

Loading

YarShev commented Nov 28, 2023 •

edited

Loading