Activate mypy in ignite.distributed

[gruebel]

Fixes #1344

Description:
I added the # type: ignore comments, where needed. Some stuff is really strange, like using @overload and the disallow_untyped_defs flag is not supported by mypy 🤯
I tried my best to add the correct types to parameters and functions, so please have a look 🙏

In general, after activating mypy for the whole project, we could try out the --strict flag, which will probably find addtional things.

[vfdev-5]

Thanks a lot @gruebel for the PR !
Make existing codebase pass mypy is something really tough. This is really helpful for the project what you are doing 💪 !
I left few comments.

In general, after activating mypy for the whole project, we could try out the --strict flag, which will probably find addtional things.

Thanks for the suggestion. Let's keep it in mind. I think making ignite.engine and ignite.metrics pass mypy will be a disaster :)

[gruebel]

I just ran mypy on ignite.metrics ~100errors 😱 ignite.engine doesn't look so bad with ~30.

I already started to work on ignite.contrib. Should I create an issue for it?

[vfdev-5]

Maybe, let's create an issue for metrics and engine.

I already started to work on ignite.contrib. Should I create an issue for it?

@gruebel thanks, however I was thinking to do that in 2 steps: 1) add typing, 2) make mypy happy.
There is already a PR on introducing typing to contrib module : #1351

If you'd like to help with reviewing it (would be awesome) and then, if you wish, you can make mypy on contrib. What do you think ?

[gruebel]

If you'd like to help with reviewing it (would be awesome) and then, if you wish, you can make mypy on contrib. What do you think ?

Yeah, sure. Didn't see there is an open PR for it.

[vfdev-5]

Thanks for the update, I left some more comments

[vfdev-5]

Same here for Optional ?

[gruebel]

same as above, because in the __init__ function of Parallel backend is None by default

ignite/ignite/distributed/launcher.py

Lines 176 to 185 in 5106738

	def __init__(
	self,
	backend: str = None,
	nproc_per_node: Optional[int] = None,
	nnodes: Optional[int] = None,
	node_rank: Optional[int] = None,
	master_addr: Optional[str] = None,
	master_port: Optional[str] = None,
	**spawn_kwargs
	):

and in the function run it calls utils.spwan

ignite/ignite/distributed/launcher.py

Line 271 in 5106738

idist.spawn(self.backend, func, args=args, kwargs_dict=kwargs, **self._spawn_params)

and in the whole Parallel self.backend is not set with a new backend, therefore it could be None till this point.

[vfdev-5]

I think there is some to fix in the code as here it shouldn't be optional and can't be None.
And ignite/ignite/distributed/launcher.py does not use _assert_backend. It is used only in

ignite/ignite/distributed/utils.py

Line 449 in b977b74

def initialize(backend: str, **kwargs: Any) -> None:

where backend is not optional.

[gruebel]

No, it is also used in the spawn function

ignite/ignite/distributed/utils.py

Lines 192 to 199 in b977b74

	def spawn(
	backend: Optional[str],
	fn: Callable,
	args: Tuple,
	kwargs_dict: Optional[Mapping] = None,
	nproc_per_node: int = 1,
	**kwargs: Any
	) -> None:

ignite/ignite/distributed/utils.py

Line 306 in b977b74

_assert_backend(backend)

there is a very long function description, which I didn't want to add here 😄

[vfdev-5]

Yes, true, that's why it was refactored into _assert_backend, but anyway in both backend can't be optional.

[gruebel]

ok, is it then ok to add here ... and self.backend is not None:?

ignite/ignite/distributed/launcher.py

Lines 276 to 278 in b977b74

	if self._spawn_params is not None:
	self.logger.info("Spawn function '{}' in {} processes".format(func, self._spawn_params["nproc_per_node"]))
	idist.spawn(self.backend, func, args=args, kwargs_dict=kwargs, **self._spawn_params)

[gruebel]

otherwise I can also put here # type: ignore

[vfdev-5]

I think there is some to fix in the code as here it shouldn't be optional and can't be None.
And ignite/ignite/distributed/launcher.py does not use _assert_backend. It is used only in

ignite/ignite/distributed/utils.py

Line 449 in b977b74

def initialize(backend: str, **kwargs: Any) -> None:

where backend is not optional.

[gruebel]

@vfdev-5 any idea about the failing tests?

[vfdev-5]

Good question. There are errors:

• mypy: https://travis-ci.org/github/pytorch/ignite/jobs/733127268#L1421
• test: https://github.com/pytorch/ignite/pull/1355/checks?check_run_id=1211799248#step:5:117

Let me restart XLA test. For mypy, maybe, we have investigate it more.

[vfdev-5]

Here it is DataLoader type without _MpDeviceLoader

[gruebel]

at this specific line you are right, but a few lines later it can be reassigned with _MpDeviceLoader and results in following error

ignite/distributed/auto.py:122: error: Incompatible types in assignment (expression has type "_MpDeviceLoader", variable has type "DataLoader[Any]")  [assignment]

[vfdev-5]

OK, what if we do something like

dataloader = DataLoader(dataset, **kwargs)  # type: DataLoader

if idist.has_xla_support and idist.backend() == idist_xla.XLA_TPU and world_size > 1:
    sampler = dataloader.sampler  # type: ignore[union-attr]
    mp_dataloader = mp_device_loader_cls(dataloader, idist.device())  # type: "_MpDeviceLoader"
    mp_dataloader.sampler = sampler  # type: ignore[attr-defined]
    return mp_dataloader

return dataloader

[gruebel]

yea, should work. let me try it.

[vfdev-5]

@gruebel could you please open a follow-up issue for #1355 (comment)

[gruebel]

@gruebel could you please open a follow-up issue for #1355 (comment)

done #1368!

[gruebel]

@vfdev-5 everything is green, except the TPU test.

[vfdev-5]

@gruebel thanks for opening the issue !

I'm trying to understand if XLA failing test is related to this PR or not, I restarted master XLA test which was green 6h ago : https://github.com/pytorch/ignite/runs/1214898148

Let's see and either we can merge this PR if it is unrelated or inspect and fix the issue...

[gruebel]

I think I found it, it was from my change. I will fix it.

[vfdev-5]

Thanks a lot @gruebel !

[gruebel]

@vfdev-5 I'm just curious, was it really helpful for you or is it more work, than doing it on your own? Because you are spending a lot of time in reviewing what me and others are doing here.

Thanks for your effort and patience 💪

[vfdev-5]

@gruebel that's a good question. For mypy it is something really helpful as we have no much experience with that and mypy reveals where the code is badly written. For other features sometimes it can be quickly to do it myself and what counts is the "community" and if it matters to help the person to learn our package and other related tech.

[trsvchn]

@vfdev-5 I really like the contributing atmosphere that you've created here :) It is very welcome for opensource beginners, (like me:) )