Neural Modules Configuration - stage 1: init parameters cleanup #309

tkornuta-nvidia · 2020-01-29T21:50:19Z

The goal of this PR is to prepare the ground for MNs configuration import-export.
The core idea is described in the Design Doc:
https://docs.google.com/document/d/1HhRzdsLO1hfiMBnR8v2_QIEeNe9m7qjERo4pgCpKhpk/edit#

The current PR is:

introduces function that extract init parameters in NeuralModule
introduces function that validates whether init parameters are of valid type
cleans all unnamed params that are passed to Neural Modules (kwargs) that enables to store in the init parameters only the ones that are required to instantiate the object.

Please note the changes are consistent with our contribution guidelines:
https://github.com/NVIDIA/NeMo/blob/master/CONTRIBUTING.md
in particular with the point:
*Minimize the use of *kwargs.

…KHTIAR Signed-off-by: Tomasz Kornuta <[email protected]>

…y, unit tests NOT working Signed-off-by: Tomasz Kornuta <[email protected]>

Signed-off-by: Tomasz Kornuta <[email protected]>

…alization Signed-off-by: Tomasz Kornuta <[email protected]>

Signed-off-by: Tomasz Kornuta <[email protected]>

lgtm-com · 2020-01-29T22:07:44Z

This pull request introduces 2 alerts when merging 44406e1 into 1fddca2 - view on LGTM.com

new alerts:

1 for Wrong name for an argument in a call
1 for Unused import

Signed-off-by: Tomasz Kornuta <[email protected]>

lgtm-com · 2020-01-30T02:02:58Z

This pull request introduces 16 alerts when merging 439c938 into 1fddca2 - view on LGTM.com

new alerts:

15 for Wrong name for an argument in a class instantiation
1 for Unused import

…gested by an old comment) Signed-off-by: Tomasz Kornuta <[email protected]>

lgtm-com · 2020-01-30T02:18:50Z

This pull request introduces 16 alerts when merging 7460430 into 1fddca2 - view on LGTM.com

new alerts:

15 for Wrong name for an argument in a class instantiation
1 for Unused import

stasbel · 2020-01-30T04:38:15Z

I think removing all **kwargs invocation everywhere is not valid. You have to rather "expand" them.
Maybe it is just me, but I dont get what you trying to accomplish here and why. Can you just briefly describe it in top-level PR message? Besides, you will need it anywhere for changelog link.

Signed-off-by: Tomasz Kornuta <[email protected]>

…DataLayer Signed-off-by: Tomasz Kornuta <[email protected]>

…to DL Signed-off-by: Tomasz Kornuta <[email protected]>

…ssing invalid parameters to init() Signed-off-by: Tomasz Kornuta <[email protected]>

CONTRIBUTING.md

lgtm-com · 2020-01-31T05:44:00Z

This pull request introduces 8 alerts when merging c2f1db8 into 1fddca2 - view on LGTM.com

new alerts:

7 for Wrong name for an argument in a class instantiation
1 for Unused import

okuchaiev

Please do not check in "examples/start_here/movie_data.txt file
See some comments inline

CONTRIBUTING.md

nemo/backends/pytorch/actions.py

nemo/core/neural_modules.py

tests/common_setup.py

Signed-off-by: Tomasz Kornuta <[email protected]>

docs/docs_zh/sources/source/tutorials/custommodules.rst

Signed-off-by: Tomasz Kornuta <[email protected]>

lgtm-com · 2020-01-31T19:33:12Z

This pull request introduces 8 alerts when merging 066a423 into 1fddca2 - view on LGTM.com

new alerts:

7 for Wrong name for an argument in a class instantiation
1 for Unused import

Signed-off-by: Tomasz Kornuta <[email protected]>

lgtm-com · 2020-01-31T19:44:36Z

This pull request introduces 8 alerts when merging daf1e7a into 1fddca2 - view on LGTM.com

new alerts:

7 for Wrong name for an argument in a class instantiation
1 for Unused import

Signed-off-by: Tomasz Kornuta <[email protected]>

Signed-off-by: Oleksii Kuchaiev <[email protected]>

lgtm-com · 2020-01-31T21:30:02Z

This pull request introduces 8 alerts when merging 62e2c64 into 1fddca2 - view on LGTM.com

new alerts:

7 for Wrong name for an argument in a class instantiation
1 for Unused import

Signed-off-by: Tomasz Kornuta <[email protected]>

lgtm-com · 2020-01-31T22:00:51Z

This pull request introduces 8 alerts when merging c8b0610 into 1fddca2 - view on LGTM.com

new alerts:

7 for Wrong name for an argument in a class instantiation
1 for Unused import

Signed-off-by: Tomasz Kornuta <[email protected]>

lgtm-com · 2020-01-31T22:43:23Z

This pull request introduces 7 alerts when merging 90e9eef into 4137c2b - view on LGTM.com

new alerts:

6 for Wrong name for an argument in a class instantiation
1 for Unused import

Signed-off-by: Tomasz Kornuta <[email protected]>

lgtm-com · 2020-01-31T23:34:48Z

This pull request introduces 6 alerts when merging 2d1a678 into 4137c2b - view on LGTM.com

new alerts:

5 for Wrong name for an argument in a class instantiation
1 for Unused import

Signed-off-by: Tomasz Kornuta <[email protected]>

tkornuta-nvidia · 2020-02-01T01:30:06Z

@blisc will update the changelog in the next PR (when will finish NM configuration - stage 2 ;) )

Signed-off-by: Tomasz Kornuta <[email protected]>

okuchaiev

Please see comments inline

okuchaiev · 2020-02-03T18:06:22Z

examples/start_here/simplest_example_configuration_import.py

@@ -0,0 +1,47 @@
+# ! /usr/bin/python


Why is this file needed? I don't see anything being imported or exported here

by "this" I mean the whole file "simplest_example_configuration_import.py"

examples/tts/configs/tacotron2.yaml

examples/tts/tts_infer.py

okuchaiev · 2020-02-03T18:31:55Z

nemo/core/neural_modules.py

+        # Return parameters.
+        return init_params
+
+    def _validate_params(self, params):


this should start with 2 underscores like "__extract_init_params"

nemo/core/neural_modules.py

tests/asr/test_asr.py

tests/asr/test_weight_share.py

Signed-off-by: Oleksii Kuchaiev <[email protected]>

Update CODEOWNERS Signed-off-by: Mark Sturdevant <[email protected]>

tkornuta-nvidia added 5 commits January 28, 2020 16:46

Deprecated rewritten, relies on wrapt package - help from Jonathan DE…

75a7d4f

…KHTIAR Signed-off-by: Tomasz Kornuta <[email protected]>

work in progress, cleaning up the arguments passed along NMs hierarch…

f5f5d24

…y, unit tests NOT working Signed-off-by: Tomasz Kornuta <[email protected]>

merged fix-cpu

603284a

Signed-off-by: Tomasz Kornuta <[email protected]>

cleared up parameters of NMs base classes, unit tests working - initi…

11e3977

…alization Signed-off-by: Tomasz Kornuta <[email protected]>

adding wrapt to test requirements

44406e1

Signed-off-by: Tomasz Kornuta <[email protected]>

tkornuta-nvidia added 3 commits January 29, 2020 15:36

Fixing unit tests after NeuralModule cleanup

b41f660

Signed-off-by: Tomasz Kornuta <[email protected]>

Revemod all kwargs from init() constructors of Neural Modules

439c938

Signed-off-by: Tomasz Kornuta <[email protected]>

Argument order fix in image_folder

1468c55

Signed-off-by: Tomasz Kornuta <[email protected]>

Adding batch_size property to DataLayer, with default value 1 (as sug…

7460430

…gested by an old comment) Signed-off-by: Tomasz Kornuta <[email protected]>

tkornuta-nvidia added 6 commits January 30, 2020 17:05

Fixed the references to local_parameters, including tutorials

3a5127e

Signed-off-by: Tomasz Kornuta <[email protected]>

Polishing of code

4a202fd

Signed-off-by: Tomasz Kornuta <[email protected]>

removed nf passed as argument to BERT in example

2e85582

Signed-off-by: Tomasz Kornuta <[email protected]>

Cleanup related to batch_size, shuffle and num_workers properties of …

ab9482b

…DataLayer Signed-off-by: Tomasz Kornuta <[email protected]>

actions fix - cleanup related to local_parameters, adding properties …

69b3852

…to DL Signed-off-by: Tomasz Kornuta <[email protected]>

Fixed or commented ASR tests - found many issues there, related to pa…

c2f1db8

…ssing invalid parameters to init() Signed-off-by: Tomasz Kornuta <[email protected]>

tkornuta-nvidia commented Jan 31, 2020

View reviewed changes

CONTRIBUTING.md Show resolved Hide resolved

okuchaiev requested changes Jan 31, 2020

View reviewed changes

__callll__ fix

0b88437

Signed-off-by: Tomasz Kornuta <[email protected]>

tkornuta-nvidia commented Jan 31, 2020

View reviewed changes

docs/docs_zh/sources/source/tutorials/custommodules.rst Outdated Show resolved Hide resolved

tkornuta-nvidia added 2 commits January 31, 2020 10:55

typo fix

3f39ad7

Signed-off-by: Tomasz Kornuta <[email protected]>

minor fixes, tests on GPU

066a423

Signed-off-by: Tomasz Kornuta <[email protected]>

extract init params -> private

daf1e7a

Signed-off-by: Tomasz Kornuta <[email protected]>

super()

67caf4f

Signed-off-by: Tomasz Kornuta <[email protected]>

okuchaiev added 2 commits January 31, 2020 13:19

fix black in actions.py

ed8d80d

Signed-off-by: Oleksii Kuchaiev <[email protected]>

Merge branch 'master' into dev-config-nm

62e2c64

tkornuta-nvidia added 2 commits January 31, 2020 13:32

Merge branch 'master' of github.com:NVIDIA/NeMo

c86ad6c

Fixed the chatbot_example

c8b0610

Signed-off-by: Tomasz Kornuta <[email protected]>

gan example fix

db7a89d

Signed-off-by: Tomasz Kornuta <[email protected]>

tkornuta-nvidia changed the title ~~[WIP] Neural Modules Configuration~~ [WIP] Neural Modules Configuration - stage 1: init parameters cleanup Jan 31, 2020

tkornuta-nvidia added 3 commits January 31, 2020 14:14

logger merge

4b1ef90

Signed-off-by: Tomasz Kornuta <[email protected]>

merge with Jason's PR

c8a14c6

Signed-off-by: Tomasz Kornuta <[email protected]>

fixe after merge

90e9eef

Signed-off-by: Tomasz Kornuta <[email protected]>

tkornuta-nvidia added 3 commits January 31, 2020 15:16

LGTM warning fix

f8cf8f8

Signed-off-by: Tomasz Kornuta <[email protected]>

tts config fix

2d1a678

Signed-off-by: Tomasz Kornuta <[email protected]>

lgtm import cleanup

75ad001

Signed-off-by: Tomasz Kornuta <[email protected]>

tkornuta-nvidia marked this pull request as ready for review January 31, 2020 23:56

tkornuta-nvidia changed the title ~~[WIP] Neural Modules Configuration - stage 1: init parameters cleanup~~ Neural Modules Configuration - stage 1: init parameters cleanup Feb 1, 2020

tkornuta-nvidia added 2 commits January 31, 2020 16:13

LGTM fix

91b36c4

Signed-off-by: Tomasz Kornuta <[email protected]>

LGTM-related fixes - wrong params fixes

c9d16b2

Signed-off-by: Tomasz Kornuta <[email protected]>

tkornuta-nvidia requested a review from okuchaiev February 1, 2020 00:21

tkornuta-nvidia and others added 2 commits January 31, 2020 17:59

updated the changelog

443b3ef

Signed-off-by: Tomasz Kornuta <[email protected]>

Merge branch 'master' into dev-config-nm

ec1f360

okuchaiev requested changes Feb 3, 2020

View reviewed changes

fixing some unittests

e13d39e

Signed-off-by: Oleksii Kuchaiev <[email protected]>

okuchaiev approved these changes Feb 3, 2020

View reviewed changes

tkornuta-nvidia merged commit 08a4c0d into master Feb 3, 2020

tkornuta-nvidia deleted the dev-config-nm branch February 3, 2020 22:35

dcurran90 pushed a commit to dcurran90/NeMo that referenced this pull request Oct 15, 2024

Update CODEOWNERS mairin for README.md (NVIDIA#309)

925d3d9

Update CODEOWNERS Signed-off-by: Mark Sturdevant <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Neural Modules Configuration - stage 1: init parameters cleanup #309

Neural Modules Configuration - stage 1: init parameters cleanup #309

tkornuta-nvidia commented Jan 29, 2020 •

edited

Loading

lgtm-com bot commented Jan 29, 2020

lgtm-com bot commented Jan 30, 2020

lgtm-com bot commented Jan 30, 2020

stasbel commented Jan 30, 2020

lgtm-com bot commented Jan 31, 2020

okuchaiev left a comment

lgtm-com bot commented Jan 31, 2020

lgtm-com bot commented Jan 31, 2020

lgtm-com bot commented Jan 31, 2020

lgtm-com bot commented Jan 31, 2020

lgtm-com bot commented Jan 31, 2020

lgtm-com bot commented Jan 31, 2020

tkornuta-nvidia commented Feb 1, 2020

okuchaiev left a comment

okuchaiev Feb 3, 2020

okuchaiev Feb 3, 2020

okuchaiev Feb 3, 2020

Neural Modules Configuration - stage 1: init parameters cleanup #309

Neural Modules Configuration - stage 1: init parameters cleanup #309

Conversation

tkornuta-nvidia commented Jan 29, 2020 • edited Loading

lgtm-com bot commented Jan 29, 2020

lgtm-com bot commented Jan 30, 2020

lgtm-com bot commented Jan 30, 2020

stasbel commented Jan 30, 2020

lgtm-com bot commented Jan 31, 2020

okuchaiev left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Jan 31, 2020

lgtm-com bot commented Jan 31, 2020

lgtm-com bot commented Jan 31, 2020

lgtm-com bot commented Jan 31, 2020

lgtm-com bot commented Jan 31, 2020

lgtm-com bot commented Jan 31, 2020

tkornuta-nvidia commented Feb 1, 2020

okuchaiev left a comment

Choose a reason for hiding this comment

okuchaiev Feb 3, 2020

Choose a reason for hiding this comment

okuchaiev Feb 3, 2020

Choose a reason for hiding this comment

okuchaiev Feb 3, 2020

Choose a reason for hiding this comment

tkornuta-nvidia commented Jan 29, 2020 •

edited

Loading