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 2: config import-export #339

Merged
merged 46 commits into from
Feb 19, 2020
Merged

Neural Modules Configuration stage 2: config import-export #339

merged 46 commits into from
Feb 19, 2020

Conversation

tkornuta-nvidia
Copy link
Contributor

@tkornuta-nvidia tkornuta-nvidia commented Feb 7, 2020 •
edited
Loading

Functionality implemented in this PR:

  • Generic export of configuration (init_params) to configuration (yml) file
  • Generic import of configuration from configuration file
  • Unit tests for both import and export functionalities
  • An example script showing how to use import/export
  • A tutorial showing how to use import/export
  • A tutorial showing how to implement a custom import/export
  • Refactor of an existing ASR example to see whether the solution fits to our needs

The import distinguishes two cases: when importing from the root NeuralModule class and from the leaf neural module.

This PR have also "initializes" the module/config/checkpoint versioning. For now I am saving in config the only available information, i.e. version of the NeMo core plus collection name (collections do not have their own versioning, yet). I am also not performing any version checking during import. Generally I think this issue should be discussed in a separate "Problem brainstorming session".

Additionally, this PR cleans up:

  • utilization of get_module() from NeuralFactory (CV related examples, all relying on ResNet, are left for now - will clean them up along with the introduction of CV collection)

@tkornuta-nvidia tkornuta-nvidia requested a review from blisc February 7, 2020 03:42
@tkornuta-nvidia
yaml added to requirements
08104d1 
Signed-off-by: Tomasz Kornuta <[email protected]>
@tkornuta-nvidia
print cleanup
d1692d6 
Signed-off-by: Tomasz Kornuta <[email protected]>
@tkornuta-nvidia
PyYAML fix
d322c8b 
Signed-off-by: Tomasz Kornuta <[email protected]>
@lgtm-com
Copy link

lgtm-com bot commented Feb 7, 2020

This pull request introduces 1 alert when merging d322c8b into 4f299f4 - view on LGTM.com

new alerts:

  • 1 for Module is imported with 'import' and 'import from'

blisc
blisc reviewed Feb 7, 2020
View reviewed changes
@tkornuta-nvidia
ruamel.yaml
215bb31 
Signed-off-by: Tomasz Kornuta <[email protected]>
@tkornuta-nvidia
import_from_config, export_to_config refactor
e269115 
Signed-off-by: Tomasz Kornuta <[email protected]>
@tkornuta-nvidia
check of header content in test
6c428a6 
Signed-off-by: Tomasz Kornuta <[email protected]>
@tkornuta-nvidia
chatbot_example2 cleanup
0cc1a22 
Signed-off-by: Tomasz Kornuta <[email protected]>
@tkornuta-nvidia
Simple example of config import/export
d39fc98 
Signed-off-by: Tomasz Kornuta <[email protected]>
@tkornuta-nvidia
merge with master
c3da23b 
Signed-off-by: Tomasz Kornuta <[email protected]>
@tkornuta-nvidia
Tutorial on configuration import and export
537872e 
Signed-off-by: Tomasz Kornuta <[email protected]>
@tkornuta-nvidia
Polishing of the example/tutorial
44cbd73 
Signed-off-by: Tomasz Kornuta <[email protected]>
@tkornuta-nvidia
merge with master
a175db6 
Signed-off-by: Tomasz Kornuta <[email protected]>
@tkornuta-nvidia
unjenkinsing jasper_an4 example
2c18ed2 
Signed-off-by: Tomasz Kornuta <[email protected]>
@tkornuta-nvidia
Merge branch 'dev-jasper-unjenkinsing' into dev-config-impoexpo
2c84d9b
@tkornuta-nvidia
jenkins fix - passing proper paths as args
0b4ce22 
Signed-off-by: Tomasz Kornuta <[email protected]>
@lgtm-com
Copy link

lgtm-com bot commented Feb 15, 2020

This pull request introduces 1 alert when merging bf1729e into 142bed9 - view on LGTM.com

new alerts:

  • 1 for Unused import

okuchaiev
okuchaiev previously approved these changes Feb 18, 2020
View reviewed changes
@okuchaiev
Copy link
Member

looks good to me, what's up with CI though?

@tkornuta-nvidia
custom import example
dfc204d 
Signed-off-by: Tomasz Kornuta <[email protected]>
@lgtm-com
Copy link

lgtm-com bot commented Feb 19, 2020

This pull request introduces 6 alerts when merging dfc204d into abaac1b - view on LGTM.com

new alerts:

  • 4 for Unused import
  • 1 for Non-callable called
  • 1 for Module is imported with 'import' and 'import from'

@tkornuta-nvidia
LGTM warnings cleanup
1d87187 
Signed-off-by: Tomasz Kornuta <[email protected]>
@lgtm-com
Copy link

lgtm-com bot commented Feb 19, 2020

This pull request introduces 2 alerts when merging 1d87187 into abaac1b - view on LGTM.com

new alerts:

  • 1 for Non-callable called
  • 1 for Module is imported with 'import' and 'import from'

@tkornuta-nvidia tkornuta-nvidia marked this pull request as ready for review February 19, 2020 03:52
@lgtm-com
Copy link

lgtm-com bot commented Feb 19, 2020

This pull request introduces 2 alerts when merging bedb48a into abaac1b - view on LGTM.com

new alerts:

  • 1 for Non-callable called
  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Feb 19, 2020

This pull request introduces 2 alerts when merging c87d2e2 into abaac1b - view on LGTM.com

new alerts:

  • 1 for Non-callable called
  • 1 for Unused import

blisc
blisc reviewed Feb 19, 2020
View reviewed changes
tests/test_neural_modules_export.py
@@ -0,0 +1,141 @@
# ! /usr/bin/python
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you rename this file? you are not testing Neural Module export, you are testing configuration export

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually you are right, was thinking about that when renamed examples/tutorial

blisc
blisc reviewed Feb 19, 2020
View reviewed changes
tests/test_neural_modules_import.py
@@ -0,0 +1,114 @@
# ! /usr/bin/python
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you rename this file too? again configuration import

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, creating a separate PR

@tkornuta-nvidia tkornuta-nvidia merged commit 1b1b821 into master Feb 19, 2020
@tkornuta-nvidia tkornuta-nvidia deleted the dev-config-impoexpo branch February 19, 2020 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@redoctopus redoctopus redoctopus left review comments

@blisc blisc blisc left review comments

@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.
4 participants
@tkornuta-nvidia @okuchaiev @redoctopus @blisc