SWA script added #945

ivashnyov · 2020-09-27T15:51:53Z

Before submitting

Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
Did you read the contribution guide?
Did you check the code style? catalyst-make-codestyle && catalyst-check-codestyle (pip install -U catalyst-codestyle).
Did you make sure to update the docs? We use Google format for all the methods and classes.
Did you check the docs with make check-docs?
Did you write any new necessary tests?
Did you add your new functionality to the docs?
Did you update the CHANGELOG?
You can use 'Login as guest' to see Teamcity build logs.

Description

Related Issue

Type of Change

Examples / docs / tutorials / contributors update
Bug fix (non-breaking change which fixes an issue)
Improvement (non-breaking change which improves an existing feature)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

pep8speaks · 2020-09-27T15:52:00Z

Hello @ivashnyov! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-10-13 21:04:26 UTC

Scitator · 2020-09-28T07:07:52Z

catalyst/dl/scripts/swa.py

+        "--models_mask", "-m", type=str, help="Pattern for models to average"
+    )
+    parser.add_argument(
+        "--save-avaraged-model",


why do we need this flag? this script should always produce averaged checkpoints, isn't it?

Scitator · 2020-09-28T07:11:50Z

catalyst/dl/utils/swa.py

+
+
+def generate_averaged_weights(
+    logdir: Path, models_mask: str, save_avaraged_model: bool = True


save_avaraged_model do we really need this flag?

Scitator

and looks like we need some tests :)

Scitator · 2020-09-28T16:20:07Z

catalyst/dl/utils/swa.py

-        torch.save(
-            averaged_dict, str(logdir / "checkpoints" / "swa_weights.pth")
-        )
+    torch.save(averaged_dict, str(logdir / "checkpoints" / "swa_weights.pth"))


let's return the model and save it outside

catalyst/dl/scripts/swa.py

Scitator · 2020-10-06T05:37:28Z

catalyst/dl/tests/test_swa.py

+
+from catalyst.dl.utils.swa import generate_averaged_weights
+
+sys.path.append(".")


why do we need it?

still the same question :)

Scitator · 2020-10-06T16:31:06Z

catalyst/dl/scripts/swa.py

+    """Builds the command line parameters."""
+    parser.add_argument("--logdir", type=Path, help="Path to models logdir")
+    parser.add_argument(
+        "--models_mask", "-m", type=str, help="Pattern for models to average"


let's simplify to --models-mask

Scitator · 2020-10-06T16:31:17Z

catalyst/dl/scripts/swa.py

+        "--models_mask", "-m", type=str, help="Pattern for models to average"
+    )
+    parser.add_argument(
+        "--save_path", type=Path, help="Path to save averaged model"


and save-path

btw, dont you mind to rename it to output-path?

catalyst/dl/scripts/swa.py

Scitator · 2020-10-07T05:43:46Z

catalyst/dl/scripts/swa.py

+        "--models-mask",
+        "-m",
+        type=str,
+        default="train*",


🤔 what do you think about *.pth? I mean, in this case we will take into account all possible checkpoints

Suggested change

default="train*",

default="*.pth",

Scitator · 2020-10-07T05:45:19Z

@ivashnyov Now tests show the error during swa step. Could you please run the first example locally and debug it?

Scitator

test error on minimal requirements

Pull request has been modified.

Scitator

@ivashnyov could you please add your description to the Changelog?

ditwoo · 2020-10-08T17:45:24Z

catalyst/dl/utils/swa.py

+def average_weights(state_dicts: List[dict]) -> OrderedDict:
+    """
+    Averaging of input weights.
+
+    Args:
+        state_dicts (List[dict]): Weights to average
+
+    Returns:
+        Averaged weights
+    """
+    # source https://gist.github.com/qubvel/70c3d5e4cddcde731408f478e12ef87b
+
+    average_dict = OrderedDict()
+    for k in state_dicts[0].keys():
+        average_dict[k] = torch.div(
+            sum(state_dict[k] for state_dict in state_dicts), len(state_dicts),
+        )
+    return average_dict


Hi, what do you think about adding state dict key check mechanics (raise an error when key is missing)?
Probably it will be informative to know that something wrong with <checkpoint-file-name>.pth.

For example you could check this - https://gist.github.com/Ditwoo/5de19670d9946c80916dee75e93ef545

@ditwoo your implementation also support fp16 variant, is it correct?

@ivashnyov could you please check it out?

Pull request has been modified.

mergify · 2020-10-11T21:05:00Z

This pull request is now in conflicts. @ivashnyov, could you fix it? 🙏

ditwoo · 2020-10-12T23:04:31Z

catalyst/dl/utils/swa.py

+    Averaging of input weights.
+
+    Args:
+        state_dicts (List[dict]): Weights to average


could you please remove type from docs (the type already specified in arguments)

ditwoo · 2020-10-12T23:04:45Z

catalyst/dl/utils/swa.py

+    Load weights of a model.
+
+    Args:
+        path (str): Path to model weights


could you please remove type from docs (the type already specified in arguments)

ditwoo · 2020-10-12T23:04:55Z

catalyst/dl/utils/swa.py

+        logdir (Path): Path to logs directory
+        models_mask (str): globe-like pattern for models to average


could you please remove type from docs (the type already specified in arguments)

also, correct me if I wrong but logdir can be a string object and everything will work fine.
so, could you please specify type as Union[str, Path].

Scitator · 2020-10-13T15:37:36Z

catalyst/dl/utils/swa.py

+    Returns:
+        Weights
+    """
+    weights = torch.load(path)


could you please reuse https://github.com/catalyst-team/catalyst/blob/master/catalyst/utils/checkpoint.py#L126
It has device handling :)

Scitator · 2020-10-13T15:40:29Z

catalyst/dl/scripts/swa.py

+
+def build_args(parser: ArgumentParser):
+    """Builds the command line parameters."""
+    parser.add_argument("--logdir", type=Path, help="Path to models logdir")


could we make --logdir optional? None by default
so you could use this script like

script --models-mask=/some/path/to/checkpoints*.pth

?

Pull request has been modified.

SWA script added

a9af7cd

ivashnyov requested review from bagxi, ditwoo and Scitator as code owners September 27, 2020 15:51

Codestyle fixed

27828ff

Scitator reviewed Sep 28, 2020

View reviewed changes

fixed pr issues

1f770dc

Scitator reviewed Sep 28, 2020

View reviewed changes

added test and fix

78ff817

Scitator reviewed Oct 6, 2020

View reviewed changes

codestyle fix

8260b0a

Scitator reviewed Oct 6, 2020

View reviewed changes

fixes

7ab43ab

Scitator reviewed Oct 7, 2020

View reviewed changes

test fixed

504f6be

Scitator approved these changes Oct 7, 2020

View reviewed changes

Scitator previously requested changes Oct 7, 2020

View reviewed changes

division fix

6482869

Ivan Ivashnev added 2 commits October 7, 2020 23:37

codestyle fixed

89d079c

fixed again

4b0ce99

Scitator previously approved these changes Oct 8, 2020

View reviewed changes

ditwoo reviewed Oct 8, 2020

View reviewed changes

check keys matching added

a4e07c5

Ivan Ivashnev added 2 commits October 12, 2020 00:06

Merge branch 'master' into master

5b00ca3

Merge branch 'master' into master

95034ca

fixed test

44f750a

ditwoo reviewed Oct 12, 2020

View reviewed changes

fixed docs

17a0ff1

ditwoo previously approved these changes Oct 13, 2020

View reviewed changes

Scitator previously requested changes Oct 13, 2020

View reviewed changes

Scitator reviewed Oct 13, 2020

View reviewed changes

fix

669bbb9

Scitator merged commit f2acebb into catalyst-team:master Oct 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SWA script added #945

SWA script added #945

ivashnyov commented Sep 27, 2020 •

edited

pep8speaks commented Sep 27, 2020 •

edited

Scitator Sep 28, 2020

Scitator Sep 28, 2020

Scitator Sep 28, 2020

Scitator left a comment

Scitator Sep 28, 2020

Scitator Oct 6, 2020

Scitator Oct 8, 2020

Scitator Oct 6, 2020

Scitator Oct 6, 2020

Scitator Oct 6, 2020

Scitator Oct 7, 2020

Scitator commented Oct 7, 2020

Scitator left a comment

Scitator left a comment

ditwoo Oct 8, 2020

Scitator Oct 9, 2020

Scitator Oct 9, 2020

mergify bot commented Oct 11, 2020

ditwoo Oct 12, 2020

ditwoo Oct 12, 2020

ditwoo Oct 12, 2020

Scitator Oct 13, 2020

Scitator Oct 13, 2020



		def generate_averaged_weights(
		logdir: Path, models_mask: str, save_avaraged_model: bool = True


		from catalyst.dl.utils.swa import generate_averaged_weights

		sys.path.append(".")

		logdir (Path): Path to logs directory
		models_mask (str): globe-like pattern for models to average

SWA script added #945

SWA script added #945

Conversation

ivashnyov commented Sep 27, 2020 • edited

Before submitting

Description

Related Issue

Type of Change

PR review

pep8speaks commented Sep 27, 2020 • edited

Comment last updated at 2020-10-13 21:04:26 UTC

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scitator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scitator commented Oct 7, 2020

Scitator left a comment

Choose a reason for hiding this comment

Scitator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Oct 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivashnyov commented Sep 27, 2020 •

edited

pep8speaks commented Sep 27, 2020 •

edited