Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 40 commits into from
Feb 3, 2020
Merged

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

merged 40 commits into from
Feb 3, 2020

Conversation

tkornuta-nvidia
Copy link
Contributor

@tkornuta-nvidia tkornuta-nvidia commented Jan 29, 2020 •
edited
Loading

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.

@tkornuta-nvidia
Deprecated rewritten, relies on wrapt package - help from Jonathan DE…
75a7d4f 
…KHTIAR

Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@tkornuta-nvidia
work in progress, cleaning up the arguments passed along NMs hierarch…
f5f5d24 
…y, unit tests NOT working

Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@tkornuta-nvidia
merged fix-cpu
603284a 
Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@tkornuta-nvidia
cleared up parameters of NMs base classes, unit tests working - initi…
11e3977 
…alization

Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@tkornuta-nvidia
adding wrapt to test requirements
44406e1 
Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@lgtm-com
Copy link

lgtm-com bot commented Jan 29, 2020

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

@tkornuta-nvidia
Fixing unit tests after NeuralModule cleanup
b41f660 
Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@tkornuta-nvidia
Revemod all kwargs from init() constructors of Neural Modules
439c938 
Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@tkornuta-nvidia
Argument order fix in image_folder
1468c55 
Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@lgtm-com
Copy link

lgtm-com bot commented Jan 30, 2020

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

@tkornuta-nvidia
Adding batch_size property to DataLayer, with default value 1 (as sug…
7460430 
…gested by an old comment)

Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@lgtm-com
Copy link

lgtm-com bot commented Jan 30, 2020

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
Copy link
Contributor

stasbel commented Jan 30, 2020

  1. I think removing all **kwargs invocation everywhere is not valid. You have to rather "expand" them.
  2. 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.

@tkornuta-nvidia
Fixed the references to local_parameters, including tutorials
3a5127e 
Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@tkornuta-nvidia
Polishing of code
4a202fd 
Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@tkornuta-nvidia
removed nf passed as argument to BERT in example
2e85582 
Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@tkornuta-nvidia
Cleanup related to batch_size, shuffle and num_workers properties of …
ab9482b 
…DataLayer

Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@tkornuta-nvidia
actions fix - cleanup related to local_parameters, adding properties …
69b3852 
…to DL

Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@tkornuta-nvidia
Fixed or commented ASR tests - found many issues there, related to pa…
c2f1db8 
…ssing invalid parameters to init()

Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@lgtm-com
Copy link

lgtm-com bot commented Jan 31, 2020

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
okuchaiev requested changes Jan 31, 2020
View reviewed changes
Copy link
Member

@okuchaiev okuchaiev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

CONTRIBUTING.md Show resolved Hide resolved
nemo/backends/pytorch/actions.py Show resolved Hide resolved
nemo/backends/pytorch/actions.py Outdated Show resolved Hide resolved
nemo/backends/pytorch/actions.py Outdated Show resolved Hide resolved
nemo/core/neural_modules.py Outdated Show resolved Hide resolved
nemo/core/neural_modules.py Outdated Show resolved Hide resolved
nemo/core/neural_modules.py Outdated Show resolved Hide resolved
nemo/core/neural_modules.py Outdated Show resolved Hide resolved
tests/common_setup.py Outdated Show resolved Hide resolved
@tkornuta-nvidia
__callll__ fix
0b88437 
Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@tkornuta-nvidia
typo fix
3f39ad7 
Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@tkornuta-nvidia
minor fixes, tests on GPU
066a423 
Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@lgtm-com
Copy link

lgtm-com bot commented Jan 31, 2020

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

@tkornuta-nvidia
extract init params -> private
daf1e7a 
Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@lgtm-com
Copy link

lgtm-com bot commented Jan 31, 2020

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

@tkornuta-nvidia
super()
67caf4f 
Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@okuchaiev
fix black in actions.py
ed8d80d 
Signed-off-by: Oleksii Kuchaiev <okuchaiev@nvidia.com>
@okuchaiev
Merge branch 'master' into dev-config-nm
62e2c64
@lgtm-com
Copy link

lgtm-com bot commented Jan 31, 2020

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

@lgtm-com
Copy link

lgtm-com bot commented Jan 31, 2020

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

@tkornuta-nvidia
gan example fix
db7a89d 
Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@tkornuta-nvidia tkornuta-nvidia changed the title [WIP] Neural Modules Configuration [WIP] Neural Modules Configuration - stage 1: init parameters cleanup Jan 31, 2020
@tkornuta-nvidia
logger merge
4b1ef90 
Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@tkornuta-nvidia
merge with Jason's PR
c8a14c6 
Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@tkornuta-nvidia
fixe after merge
90e9eef 
Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@lgtm-com
Copy link

lgtm-com bot commented Jan 31, 2020

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

@tkornuta-nvidia
LGTM warning fix
f8cf8f8 
Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@tkornuta-nvidia
tts config fix
2d1a678 
Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@tkornuta-nvidia
lgtm import cleanup
75ad001 
Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@lgtm-com
Copy link

lgtm-com bot commented Jan 31, 2020

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

@tkornuta-nvidia tkornuta-nvidia marked this pull request as ready for review January 31, 2020 23:56
@tkornuta-nvidia 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
LGTM fix
91b36c4 
Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@tkornuta-nvidia
LGTM-related fixes - wrong params fixes
c9d16b2 
Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@tkornuta-nvidia
Copy link
Contributor Author

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

tkornuta-nvidia and others added 2 commits January 31, 2020 17:59
@tkornuta-nvidia
updated the changelog
443b3ef 
Signed-off-by: Tomasz Kornuta <tkornuta@nvidia.com>
@okuchaiev
Merge branch 'master' into dev-config-nm
ec1f360
okuchaiev
okuchaiev requested changes Feb 3, 2020
View reviewed changes
Copy link
Member

@okuchaiev okuchaiev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see comments inline

examples/start_here/simplest_example_configuration_import.py
@@ -0,0 +1,47 @@
# ! /usr/bin/python
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

examples/tts/configs/tacotron2.yaml Show resolved Hide resolved
examples/tts/tts_infer.py Show resolved Hide resolved
nemo/core/neural_modules.py
# Return parameters.
return init_params

def _validate_params(self, params):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should start with 2 underscores like "__extract_init_params"

nemo/core/neural_modules.py Outdated Show resolved Hide resolved
tests/asr/test_asr.py Outdated Show resolved Hide resolved
tests/asr/test_asr.py Outdated Show resolved Hide resolved
tests/asr/test_asr.py Outdated Show resolved Hide resolved
tests/asr/test_asr.py Outdated Show resolved Hide resolved
tests/asr/test_weight_share.py Outdated Show resolved Hide resolved
@okuchaiev
fixing some unittests
e13d39e 
Signed-off-by: Oleksii Kuchaiev <okuchaiev@nvidia.com>
@tkornuta-nvidia tkornuta-nvidia merged commit 08a4c0d into master Feb 3, 2020
@tkornuta-nvidia 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
@markstur
Update CODEOWNERS mairin for README.md (NVIDIA#309)
925d3d9 
Update CODEOWNERS

Signed-off-by: Mark Sturdevant <mark.sturdevant@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@okuchaiev okuchaiev okuchaiev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tkornuta-nvidia @stasbel @okuchaiev