Codecov Report

Merging#4683 (490778c) intomaster (8e1190c) willdecrease coverage by13.12%.
The diff coverage is67.93%.

@@             Coverage Diff             @@##           master    #4683       +/-   ##===========================================- Coverage   85.28%   72.15%   -13.13%===========================================  Files         259      259                 Lines       19378    19496      +118     ===========================================- Hits        16527    14068     -2459- Misses       2851     5428     +2577

📣 Codecov can now indicate which changes are the most critical in Pull Requests.Learn more

Haven't taken a closer look at the implementation details, but do you have any benchmarks or performance measurements to compare with master?

@noloerino@pyrito

Sadly no, and I’d appreciate some suggestions on what code to run.

I spent a while today trying to get a script that showcases the performance here without breaking anything in Modin, but I failed. Getting a reproducer is hard for a few reasons.

For one thing, this optimization is only useful for unusual cases like in#4493 where the partitions' call queues include costly operations. When there is no call queue, the partitions will execute all dataframe functions eagerly, simultaneously calculating shapes. The call queues are generally meant to carry cheap operations like transpose and reindexing, but thereproducer in that issue has a frame that is very expensive to serialize, so that even the transpose was expensive. There the slow code was in_copartition, which unnecessarily calculated the widths of the base frame.#4495 fixed that unnecessary recalculation, so that script no longer works. Also, everyPandasDataFrame computes all the lengths when it filters empty subframes as soon as it's constructedhere, so any Modin dataframe at rest already knows its partition shapes.

Looking at all the serial shape computations I listedhere, most are in internal length computations. One is_copartition, and I spent a while trying to get around the cache fix in#4495 with a pair of frames that really needed copartitioning, but in that case themap_axis_partitions in_copartition triggers parallel computation. The last type of length computation is inapply_func_to_indices_both_axis, which as far as I can tell is only used inmelt. We could try engineering an example that bypasses the cache formelt, but I don't think it's worth the time...

I think it's good practice to get multiple ray objects in parallel (see alsothis note about a similar improvement in_to_pandas). Also, if our caches fail for any reason later on, we can have faster length computation as a backup.

This adds a certain bit of complexity (judging by the number of lines change, haven't looked at the diff yet), and I haven't yet seen any performance proof for that. I would like to see some measurements before increasing our (already huge) codebase...

@vnlitvinov that makes sense, I'll look into coming up with concrete benchmarks.

@pyrito please have a look athttps://github.com/vnlitvinov/modin/tree/speedup-masking and#4726, it might be doing somewhat the same in terms of getting the sizes in parallel

Related discussion on handling metadata (index and columns) in#3673.