REFACTOR-#4631: Add mypy checks for modin.distributed

[rosdyana]

Signed-off-by: Rosdyana Kusuma rosdyana.kusuma@gmail.com

What do these changes do?

• commit message follows format outlined here
• 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 Add mypy checks for modin.distributed #4631
• tests added and passing
• module layout described at docs/development/architecture.rst is up-to-date

[mvashishtha]

@rosdyana thank you so much for making this PR. I have left some suggestions inline.

There will likely be some errors once you accept my suggestion to edit the mypy.ini file so it includes modin/distributed/dataframe/pandas/partitions.py. Please try to address them and check your work locally by running mypy --config-file mypy.ini modin/distributed. If you have any questions or you get stock, please comment here.

[mvashishtha]

@rosdyana in order to pass the commit lint, you will have to amend your first commit so that it says something like, "FIX-#4631: Add mypy checks for modin.distributed."

You should be able to amend your first commit with:

1. git rebase -i d5bd6376d5b7735c4c676e74bd73ef36edcdfdff~1
2. You should then get an editable list of actions for each commit. Choose "r" for reword for your first commit, then "p" for each of the other commits to keep them on top of the first one.
   
3. Save and exit the editable list of actions.
4. You should get an editor again asking you to reword your initial commit. Do so.
   
5. Resolve merge conflicts

[mvashishtha]

@rosdyana it looks like you've added commit e904736 with the correct formatting, but your first commit still has the incorrect formatting. Your commits as they are will still fail the CI. Could you please try rewording your first commit?

Meanwhile, I'll respond to the open inline comments. Please ping this thread if I haven't responded within 24 hours from now.

[mvashishtha]

@rosdyana just FYI, next time you want to accept someone's suggestion on a GitHub PR, I highly recommend using the "commit suggestion" if you want to accept one suggestion, or the "add suggestion to batch" button if you have multiple suggestions. That usually reduces the amount of work you have to do and makes it easier for everyone to track what's happening with the PR.

[rosdyana]

@rosdyana in order to pass the commit lint, you will have to amend your first commit so that it says something like, "FIX-#4631: Add mypy checks for modin.distributed."

You should be able to amend your first commit with:

1. git rebase -i d5bd6376d5b7735c4c676e74bd73ef36edcdfdff~1
2. You should then get an editable list of actions for each commit. Choose "r" for reword for your first commit, then "p" for each of the other commits to keep them on top of the first one.
   
3. Save and exit the editable list of actions.
4. You should get an editor again asking you to reword your initial commit. Do so.
   
5. Resolve merge conflicts

I tried to amend that commit, but it doesn't work, I have no idea. Do you mind if I re-create the PR?

[mvashishtha]

@rosdyana what's the error that you got? Did you try to "reword" the first commit and "squash" or "pick" the other commits? Rebasing can be hard, but it's better for history keeping and easier for everyone involved with the PR if we can keep all the comment history in a single PR.

[rosdyana]

@rosdyana what's the error that you got? Did you try to "reword" the first commit and "squash" or "pick" the other commits? Rebasing can be hard, but it's better for history keeping and easier for everyone involved with the PR if we can keep all the comment history in a single PR.

Yes, I did "reword" for the first commit, and then choose "pick" for the other commits. Should I "squash" the other commits?

[mvashishtha]

@rosdyana I'm going to remove the commit requirement in #4982. Please don't worry about the commit message or rebasing at all for now.

[mvashishtha]

@rosdyana I left some more comments. Please respond here or in the comment threads if you have any questions or concerns. Also, please be sure to run mypy --config-file=mypy.ini modin/distributed/ locally from the modin root to check whether mypy is passing. If it fails and you can't fix the failures, just say so here and we'll try to come to a resolution.

Once again, there's no need to rebase or amend your first commit.

[rosdyana]

@rosdyana I left some more comments. Please respond here or in the comment threads if you have any questions or concerns. Also, please be sure to run mypy --config-file=mypy.ini modin/distributed/ locally from the modin root to check whether mypy is passing. If it fails and you can't fix the failures, just say so here and we'll try to come to a resolution.

Once again, there's no need to rebase or amend your first commit.

Hi @mvashishtha the mypy returns this error

modin/distributed/dataframe/pandas/partitions.py:168:5: error: Statement is unreachable  [unreachable]
Found 1 error in 1 file (checked 4 source files)

modin/modin/distributed/dataframe/pandas/partitions.py

Line 168 in 88ef64c

assert factory.io_cls.frame_cls._partition_mgr_cls is not None

Should we ignore it? or we need to solve it?

[codecov]

Codecov Report

Merging #5109 (88ef64c) into master (4bcce6e) will decrease coverage by 28.05%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           master    #5109       +/-   ##
===========================================
- Coverage   84.56%   56.50%   -28.06%     
===========================================
  Files         256      256               
  Lines       19347    18709      -638     
===========================================
- Hits        16361    10572     -5789     
- Misses       2986     8137     +5151     

Impacted Files	Coverage Δ
modin/distributed/dataframe/pandas/partitions.py	0.00% <0.00%> (-89.48%)	⬇️
modin/distributed/__init__.py	0.00% <0.00%> (-100.00%)	⬇️
modin/experimental/core/__init__.py	0.00% <0.00%> (-100.00%)	⬇️
modin/_compat/core/latest/base_io.py	0.00% <0.00%> (-100.00%)	⬇️
modin/experimental/pandas/__init__.py	0.00% <0.00%> (-100.00%)	⬇️
modin/experimental/sklearn/__init__.py	0.00% <0.00%> (-100.00%)	⬇️
modin/experimental/xgboost/__init__.py	0.00% <0.00%> (-100.00%)	⬇️
modin/distributed/dataframe/__init__.py	0.00% <0.00%> (-100.00%)	⬇️
modin/_compat/pandas_api/latest/utils.py	0.00% <0.00%> (-100.00%)	⬇️
modin/_compat/pandas_api/latest/window.py	0.00% <0.00%> (-100.00%)	⬇️
... and 152 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[mvashishtha]

@rosdyana I think we need some more mypy annotations before we can convince mypy that all those classes are not None-- right now I think it thinks that some of them are None, so the assertions will fail and we'll never reach the code below. Can you add # type: ignore[unreachable] to the end of line 168 (the last assertion)?

[pyrito]

@rosdyana could you rebase on the latest master? There were some changes that broke CI earlier but it should be fixed now.

[mvashishtha]

@rosdyana @pyrito no need to rebase on master. I don't think the breaking changes are in the branch for this PR. @rosdyana you should be able to just push an empty commit to get testing working again:

git commit -sam "Empty commit to trigger CI." --no-verify --allow-empty && git push

[rosdyana]

Hey, @mvashishtha @pyrito let me know if I need to change anything else. Thanks!

[vnlitvinov]

@rosdyana please take a look at CI failure here: https://github.com/modin-project/modin/actions/runs/3289605374/jobs/5432602430

Maybe we should define that Union under if TYPE_CHECKING, and leave it as Any otherwise, smth like

if TYPE_CHECKING:
    from modin.core.execution.ray.implementations.pandas_on_ray.partitioning import (
        PandasOnRayDataframePartition,
    )
    from modin.core.execution.dask.implementations.pandas_on_dask.partitioning.partition import (
        PandasOnDaskDataframePartition,
    )
    PartitionUnionType =  Union[PandasOnRayDataframePartition, PandasOnDaskDataframePartition]
else:
    from typing import Any
    PartitionUnionType =  Any

# skip many lines of code

    def _unwrap_partitions() -> list:
        [p.drain_call_queue() for p in modin_frame._partitions.flatten()]
    
        def get_block(partition: PartitionUnionType) -> np.ndarray:

(untested)

[vnlitvinov]

LGTM, thanks a lot @rosdyana!

[pyrito]

Thanks for your contributions here @rosdyana !!

[mvashishtha]

+1 @rosdyana thank you for your contribution. Please continue reach out on GitHub, slack, or discuss, if you would like to contribute more.