-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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.
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
[Core] return_config=True now extracts just config, not full tarfile #6346
Conversation
Signed-off-by: smajumdar <[email protected]>
@@ -372,7 +372,7 @@ def restore_from( | |||
loaded_params = super().load_config_and_state_dict( | |||
calling_cls, restore_path, override_config_path, map_location, strict, return_config, trainer, | |||
) | |||
if not isinstance(loaded_params, tuple): | |||
if not isinstance(loaded_params, tuple) or return_config is True: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not isinstance(loaded_params, tuple) or return_config is True: | |
if not isinstance(loaded_params, tuple) or return_config: |
Could we not just simplify to this? Seems it's always a bool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer explicit checks generally, even for bool assertion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for doing this!
…VIDIA#6346) Signed-off-by: smajumdar <[email protected]> Signed-off-by: shane carroll <[email protected]>
* [Core] return_config=True now extracts just config, not full tarfile (#6346) Signed-off-by: smajumdar <[email protected]> Signed-off-by: shane carroll <[email protected]> * specaug speedup Signed-off-by: shane carroll <[email protected]> * comments Signed-off-by: shane carroll <[email protected]> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Signed-off-by: shane carroll <[email protected]> --------- Signed-off-by: smajumdar <[email protected]> Signed-off-by: shane carroll <[email protected]> Co-authored-by: Somshubra Majumdar <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
…VIDIA#6346) Signed-off-by: smajumdar <[email protected]> Signed-off-by: hsiehjackson <[email protected]>
* [Core] return_config=True now extracts just config, not full tarfile (NVIDIA#6346) Signed-off-by: smajumdar <[email protected]> Signed-off-by: shane carroll <[email protected]> * specaug speedup Signed-off-by: shane carroll <[email protected]> * comments Signed-off-by: shane carroll <[email protected]> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Signed-off-by: shane carroll <[email protected]> --------- Signed-off-by: smajumdar <[email protected]> Signed-off-by: shane carroll <[email protected]> Co-authored-by: Somshubra Majumdar <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Signed-off-by: hsiehjackson <[email protected]>
What does this PR do ?
Make return_config=True now only extract out the yaml files from the tarfile, preventing opening and file extractions of all files (should significantly speedup return_config=True for Billion parameter models).
Collection: [Core, ASR, NLP, TTS]
Changelog
Usage
Before your PR is "Ready for review"
Pre checks:
PR Type: