FIX-#6594: fix usage of Modin objects inside UDFs for apply

[anmyachev]

What do these changes do?

The PR also fixes #4919

• 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 BUG: Using Modin objects within an apply fails, with unclear error message #6594
• tests added and passing
• module layout described at docs/development/architecture.rst is up-to-date

[anmyachev]

@Retribution98 could you check why test__reduce__ failed on unidist?

The error: KeyError: <weakref at 0x0000025BB1211220; to 'MasterDataID' at 0x0000025BB0F900A0> (that crashed python).

[anmyachev]

@dchigarev could you approve again?

[dchigarev]

why would we ever want to recreate modin objects on a worker? wouldn't it make applying any method to them super slow anyway?

[anmyachev]

why would we ever want to recreate modin objects on a worker?

The main goal is to avoid materializing the dataframe in the main process and transfer this operation to the worker process.

wouldn't it make applying any method to them super slow anyway?

The only operation needed for this is taking the object by reference (get operation) to perform the conversion to pandas and perform any operations on the pandas object.

[dchigarev]

could you please also elaborate on how the changes fixes the problem?

[anmyachev]

could you please also elaborate on how the changes fixes the problem?

Instead of creating a Modin object in the worker process and performing operations on it, a Modin object will be created, converted to a pandas, and operations will in turn be performed on the pandas object.

[dchigarev]

correct me if I'm wrong, but my understanding is that we use _inflate_light() only when we unpickle a modin.DataFrame from the plasma storage (as if we wanted to read it from the disk, we would use _inflate_full()), so my question is, whether it makes sense to transfer the query compiler to workers and only then call .to_pandas()? Isn't calling .to_pandas() several times on every worker is more expensive than calling it only once in the main process when serializing? What are the benefits of using ._inflate_light()?

[dchigarev]

this is the only question that bothers me, otherwise, the PR looks good

[anmyachev]

Isn't calling .to_pandas() several times on every worker is more expensive than calling it only once in the main process when serializing?

It may be more expensive, but this method allows calculations to run asynchronously, which mitigates this problem (partially) given that worker processes tend to be under-loaded. On the other hand, the memory consumption on the other hand will be much greater, I believe that you are right and we need to make the call in the main process.

[anmyachev]

@dchigarev I suppose we can leave the current implementation, but for those cases where to_pandas is called several times (for example, in different apply that go through preprocess), enable a mode in which to_pandas is called once in the main process.

[dchigarev]

I suppose we can leave the current implementation

You mean ._inflate_light()? Where else do we use it? If we are certain that we always want for dataframes to be persistently pickled, then I suppose we shouldn't leave this implementation. There's still a possibility that there are other places in our project where we submit kernels, but don't do this config variables manipulation, which will result into that the ._inflate_light() implementation will be called against our will.

[dchigarev]

my take is, that we either should drop the ._inflate_light() implementation at all, or remove these config variable manipulations and always execute .__reduce__() in accordance with what a user set in PersistentPickle variable. At this point I'm ok with both of the options, it's up to you to decide @anmyachev

[anmyachev]

You mean ._inflate_light()?

Yes

Where else do we use it?

In a situation like this (it seems to me that this case is almost never seen, if you think so, then I’ll delete _inflate_light):

modin/modin/pandas/test/dataframe/test_pickle.py

Line 46 in 41ecc92

other = pickle.loads(pickle.dumps(modin_df))

[dchigarev]

on the second thought, it maybe it make sense keeping inflate_light, let's leave it for now

[anmyachev]

@dchigarev unidist stuck on test_io.py again.

[anmyachev]

@dchigarev ready to merge