Add kwargs to idist.barrier #2310

fco-dv · 2021-11-04T22:11:55Z

Fixes #2213

Description:

Add kwargs to idist.barrier for native , xla and horovod backends. Need to add relevant example code.

Check list:

New tests are added (if a new feature is added)
New doc strings: description and/or example code are in RST format
Documentation is updated (if required)

…mentation update

KickItLikeShika

Thanks for the PR @fco-dv! Just left a single comment

KickItLikeShika · 2021-11-05T11:07:36Z

ignite/distributed/comp_models/base.py

+    def _check_barrier_fn_kwargs(self, barrier_fn: Callable, kwargs_dict: Dict[str, Any]) -> Dict[str, Any]:
+        bnd_keys = set()
+        for param in signature(barrier_fn).parameters.values():
+            if param.kind == param.POSITIONAL_OR_KEYWORD:


Can't we simplify this a bit? I think we can do this inside the loop, for example in the else we delete the key directly maybe? and make a bool flag maybe and raise the warning

Thanks for your comment, @KickItLikeShika , I will try to simplify this

sdesrozis · 2021-11-05T18:17:38Z

ignite/distributed/comp_models/base.py

-            if param.kind == param.POSITIONAL_OR_KEYWORD:
-                bnd_keys.add(param.name)
-        extra_keys = sorted(list(set(kwargs_dict) - bnd_keys))
+        fn_params_name = set(


I’m wondering whether comprehension list would be more simple.

I think the method bind of the class Signature could help.

sdesrozis · 2021-11-05T18:20:16Z

ignite/distributed/comp_models/horovod.py

-        def barrier(self) -> None:
+        def barrier(self, **kwargs: Any) -> None:
+            kwargs = self._check_barrier_fn_kwargs(barrier_fn=hvd.allreduce, kwargs_dict=kwargs)
+            if "tensor" in kwargs:


Why redefining these arguments and not simply avoid it ? IMO it’s confusing.

sdesrozis · 2021-11-05T18:21:55Z

ignite/distributed/comp_models/horovod.py

            # https://github.com/horovod/horovod/issues/159#issuecomment-424834603
            # hvd.allreduce(torch.tensor(0, device=self.device()), name="barrier")
-            hvd.allreduce(torch.tensor(0, device="cpu"), name="barrier")
+            hvd.allreduce(tensor=torch.tensor(0, device="cpu"), name="barrier", **kwargs)


Note that a barrier function was recently introduced. We could use it in a next PR (and handle older version).

sdesrozis · 2021-11-05T18:23:08Z

ignite/distributed/comp_models/xla.py

-            xm.rendezvous("barrier")
+        def barrier(self, **kwargs: Any) -> None:
+            kwargs = self._check_barrier_fn_kwargs(barrier_fn=xm.rendezvous, kwargs_dict=kwargs)
+            if "tag" in kwargs:


Same remark than above

sdesrozis · 2021-11-05T18:23:41Z

ignite/distributed/utils.py

+
+            - | "horovod" : ``average`` (default, None), ``compression`` (default, Compression.none),
+              | ``op`` (default, None), ``prescale_factor`` (default, 1.0), ``postscale_factor`` (default, 1.0),
+              | ``process_set`` (default, global_process_set).


Redefinition is confusing IMO

sdesrozis · 2021-11-05T18:26:52Z

ignite/distributed/comp_models/base.py

@@ -275,8 +277,24 @@ def _do_all_gather(self, tensor: torch.Tensor) -> torch.Tensor:
    def _do_broadcast(self, tensor: torch.Tensor, src: int) -> torch.Tensor:
        pass

+    def _check_barrier_fn_kwargs(self, barrier_fn: Callable, kwargs_dict: Dict[str, Any]) -> Dict[str, Any]:


This function is more general than its name suggests. You did a signature checker. It could be called check_method_args and note that type could be also checked.

sdesrozis · 2021-11-05T18:34:35Z

Hey @fco-dv I left a few comments.

* Add doctests for RMSE and MeanPairwiseDistance * Add doctests for RMSE and MeanPairwiseDistance * Make metric name consistent

Co-authored-by: Desroziers <[email protected]>

…actionalAbsoluteError``, ``CanberraMetric``, ``GeometricMeanAbsoluteError``) (pytorch#2323) * added doctest for contrib regression metrics * removed Engine definition and variable name changes * added import of contrib.regression Co-authored-by: Sylvain Desroziers <[email protected]>

* pytorch#2313 fix bug in StateParamScheduler attach method * pytorch#2313 add assert if KeyError is raised * pytorch#2313 update test_multiple_scheduler_with_save_history Co-authored-by: vfdev <[email protected]>

Co-authored-by: Desroziers <[email protected]>

* pytorch#2090 make work EMAHandler with StateParamScheduler * pytorch#2295 update docstring for parameter `create_new` * pytorch#2295 fixing attach method logic and update associated tests * pytorch#2295 fix unused import / remove useless test * pytorch#2295 fix tests * pytorch#2295 rm else statement * Update ignite/handlers/state_param_scheduler.py Co-authored-by: vfdev <[email protected]> * Update ignite/handlers/state_param_scheduler.py Co-authored-by: vfdev <[email protected]> * pytorch#2295 update tests assert messages * pytorch#2295 update tests, add working example with EMAHandler Co-authored-by: vfdev <[email protected]>

fco-dv added 7 commits October 31, 2021 21:50

pytorch#2213 add kwargs to idist.barrier => needs more tests and docu…

ccedbb0

…mentation update

rm Mapping import

b1cbd1c

pytorch#2213 pass kwargs in xla and hvdconftest, add tests

596b29e

pytorch#2213 fix tests

beca8c9

pytorch#2213 add tests on warning

9b4f70d

pytorch#2213 fix test

31488ce

pytorch#2213 update doc

68393f1

github-actions bot added the module: distributed Distributed module label Nov 4, 2021

pytorch#2213 fix docstring

5aa6ade

KickItLikeShika reviewed Nov 5, 2021

View reviewed changes

vfdev-5 requested a review from sdesrozis November 5, 2021 11:51

pytorch#2213 refactor _check_barrier_fn_kwargs

6419d75

sdesrozis reviewed Nov 5, 2021

View reviewed changes

DevPranjal and others added 11 commits November 23, 2021 23:11

Doctests for RMSE and MeanPairwiseDistance (pytorch#2307)

0fa0299

* Add doctests for RMSE and MeanPairwiseDistance * Add doctests for RMSE and MeanPairwiseDistance * Make metric name consistent

disabled apex tests (pytorch#2308)

e723e60

refactor metrics - add doctest for psnr (pytorch#2311)

d2403d4

add doctest for nlp metrics (pytorch#2317)

d0b85af

Co-authored-by: Desroziers <[email protected]>

[skip ci] add doctest for CohenKappa metric (pytorch#2321)

9e66d46

[skip ci] add doctest for regression metrics (pytorch#2324)

c7d2142

pytorch#2313 fix bug in StateParamScheduler attach method (pytorch#2316)

9d87c9d

* pytorch#2313 fix bug in StateParamScheduler attach method * pytorch#2313 add assert if KeyError is raised * pytorch#2313 update test_multiple_scheduler_with_save_history Co-authored-by: vfdev <[email protected]>

add run_code_style for windows (pytorch#2329)

32515c3

Co-authored-by: Desroziers <[email protected]>

[skip ci] Add doctest for LinearCyclicalScheduler (pytorch#2327)

ed2f0ee

[skip ci] fix version extraction (pytorch#2331)

5dc455b

fco-dv and others added 2 commits November 23, 2021 23:11

pytorch#2213 refactor signature checker / update tests

b5a9ac6

fco-dv marked this pull request as draft November 23, 2021 22:12

Merge branch 'master' into kwargs_idist_barrier

ab9a74d

fco-dv closed this Nov 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add kwargs to idist.barrier #2310

Add kwargs to idist.barrier #2310

fco-dv commented Nov 4, 2021

KickItLikeShika left a comment

KickItLikeShika Nov 5, 2021

fco-dv Nov 5, 2021

sdesrozis Nov 5, 2021 •

edited

Loading

sdesrozis Nov 5, 2021

sdesrozis Nov 5, 2021

sdesrozis Nov 5, 2021

sdesrozis Nov 5, 2021

sdesrozis Nov 5, 2021

sdesrozis commented Nov 5, 2021

Add kwargs to idist.barrier #2310

Add kwargs to idist.barrier #2310

Conversation

fco-dv commented Nov 4, 2021

KickItLikeShika left a comment

Choose a reason for hiding this comment

KickItLikeShika Nov 5, 2021

Choose a reason for hiding this comment

fco-dv Nov 5, 2021

Choose a reason for hiding this comment

sdesrozis Nov 5, 2021 • edited Loading

Choose a reason for hiding this comment

sdesrozis Nov 5, 2021

Choose a reason for hiding this comment

sdesrozis Nov 5, 2021

Choose a reason for hiding this comment

sdesrozis Nov 5, 2021

Choose a reason for hiding this comment

sdesrozis Nov 5, 2021

Choose a reason for hiding this comment

sdesrozis Nov 5, 2021

Choose a reason for hiding this comment

sdesrozis commented Nov 5, 2021

sdesrozis Nov 5, 2021 •

edited

Loading