Ignite: MyPy: improve ignite.distributed module

[vfdev-5]

🚀 Feature

Currently, mypy ignores all errors for all modules. We have to rework our typing such that mypy checks the code.
In this issue, let's improve https://github.com/pytorch/ignite/tree/master/ignite/distributed module such that mypy passes on it.

For Hacktoberfest contributors, feel free to ask questions for details if any and say that you would like to tackle the issue.
Please, take a look at CONTRIBUTING guide.

[gruebel]

If it's ok, I would continue with this module 💪

[vfdev-5]

@gruebel sure !

[gruebel]

Due to this definition here
https://github.com/pytorch/ignite/blob/20167fe587cb106db59dd5123b6a02aaacd5633a/ignite/distributed/comp_models/base.py#L82-L85
mypy doesn't allow to add extra parameters in other classes like
https://github.com/pytorch/ignite/blob/20167fe587cb106db59dd5123b6a02aaacd5633a/ignite/distributed/comp_models/native.py#L259-L271
it results in

ignite/distributed/comp_models/native.py:260: error: Signature of "spawn" incompatible with supertype "ComputationModel"  [override]

To fix it I would need to change the implementation to the original definition spawn(*args, **kwargs)

[vfdev-5]

Intersting. What kind of change it would require? Maybe, then the problem will be with other implementations of spawn for Horovod, XLA comp models...
https://github.com/pytorch/ignite/blob/20167fe587cb106db59dd5123b6a02aaacd5633a/ignite/distributed/comp_models/horovod.py#L118-L127

[gruebel]

yeah, sorry. The problem relates to all three Horovod, XLA and native. I could change the function definition to

@staticmethod
@abstractmethod
def spawn(
    fn: Callable,
    args: Tuple,
    kwargs_dict: Optional[Mapping],
    nproc_per_node: int,
    **kwargs: Any
):

that's what they all have in common and extract the extra keyword args in the different implementations.

[vfdev-5]

I wonder if we can not use somehow @overload decorator for that ?

[gruebel]

As far as I understand @overload I would still need to define the function spawn(*args, **kwargs) in each implementation and put their the main code

 @staticmethod 
 @overload
 def spawn( 
     fn: Callable, 
     args: Tuple, 
     kwargs_dict: Optional[Mapping] = None, 
     nproc_per_node: int = 1, 
     nnodes: int = 1, 
     node_rank: int = 0, 
     master_addr: str = "127.0.0.1", 
     master_port: int = 2222, 
     backend: str = "nccl", 
     **kwargs: Any 
 ) -> None: 
     ...

 @staticmethod 
 def spawn(*args, **kwargs) -> None:
     [actual implementation comes here]

No sure, if it is that beneficial, but it would be possible.

[gruebel]

This is also problematic
https://github.com/pytorch/ignite/blob/74531d908296ccd270721ef37f4e02752a3906bf/ignite/distributed/auto.py#L269-L278
it results in

ignite/distributed/auto.py:277: error: Argument 1 to "__init__" of "DistributedSampler" has incompatible type "Sampler[Any]"; expected "Dataset[Any]"  [arg-type]

Can I change the type to Dataset or is it needed to be Sampler?

[vfdev-5]

that's true that there is a type mismatch between this code and base code
pytorch. However, it should be sampler.
Can we put an ignore here and for spawn now and maybe resolve them later?

On Sun, Oct 4, 2020, 16:32 Anton Grübel [e-mail] wrote:

This is also problematic

https://github.com/pytorch/ignite/blob/74531d908296ccd270721ef37f4e02752a3906bf/ignite/distributed/auto.py#L269-L278
it results in

ignite/distributed/auto.py:277: error: Argument 1 to "__init__" of "DistributedSampler" has incompatible type "Sampler[Any]"; expected "Dataset[Any]" [arg-type]

Can I change the type to Dataset or is it needed to be Sampler?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/pytorch/ignite/issues/1344#issuecomment-703264356,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AASYOHZA55KRQCA4GCHK3I3SJCBQXANCNFSM4SDIJ4DQ
.