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.

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

Use module-based k2 import guard #6006

Merged
merged 3 commits into from
Feb 13, 2023

Conversation

artbataev
Copy link
Collaborator

@artbataev artbataev commented Feb 13, 2023

Signed-off-by: Vladimir Bataev [email protected]

What does this PR do ?

Simplify k2 library import guard: use module-based approach.

Advantages:

  • less code is required for using guard
  • no need to suppress linters (isort, etc.)

Collection: [ASR]

Changelog

  • Add specific line by line info of high level changes in this PR.

Usage

from nemo.core.utils.k2_guard import k2  # import k2 with performing all required checks

Before your PR is "Ready for review"

Pre checks:

  • Make sure you read and followed Contributor guidelines
  • Did you write any new necessary tests?
  • Did you add or update any necessary documentation?
  • Does the PR affect components that are optional to install? (Ex: Numba, Pynini, Apex etc)
    • Reviewer: Does the PR have correct import guards for all optional libraries?

PR Type:

  • New Feature
  • Bugfix
  • Documentation

If you haven't finished some of the above items you can still open "Draft" PR.

Who can review?

Anyone in the community is free to review the PR once the checks have passed.
Contributor guidelines contains specific people who can review PRs to various areas.

Additional Information

  • Related to # (issue)

Signed-off-by: Vladimir Bataev <[email protected]>
@artbataev artbataev requested a review from GNroy February 13, 2023 14:06
@github-actions github-actions bot added ASR core Changes to NeMo Core labels Feb 13, 2023
GNroy
GNroy previously approved these changes Feb 13, 2023
Copy link
Collaborator

@GNroy GNroy left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Vladimir Bataev <[email protected]>
SeanNaren
SeanNaren previously approved these changes Feb 13, 2023
Copy link
Collaborator

@SeanNaren SeanNaren left a comment

Choose a reason for hiding this comment

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

LGTM!

try:
import k2
if not package_available("k2"):
raise ModuleNotFoundError(K2_INSTALLATION_MESSAGE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

How is it an optional dependency if merely importing it will raise an error ? K2 is not installed by default, the check should be don't at init inside of where it will be used rather than during simply import of a file.

Copy link
Collaborator

@titu1994 titu1994 Feb 13, 2023

Choose a reason for hiding this comment

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

Currently, k2 is not included in the import path of init.py files for this reason, but at least the check is done at init of the class. This instead puts it in the import path so if user simply imports the file it will crash with helpful error.

Numba checks specifically defers this crash to the moment user actually tries to use the module rather than simply import the module

Copy link
Collaborator Author

@artbataev artbataev Feb 13, 2023

Choose a reason for hiding this comment

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

Importing k2 raises an error only when importing k2_guard of k2 from k2_guard
k2_guard will not be imported and cached.

So, every module that tries to import k2 from k2_guard will raise an error (if no k2 / incorrect version) and will not be initialized (no classes in this module).
As a result, importing from any of the modules in nemo, that depend on k2 (maybe not directly) will result in error with the necessary message, which is a correct behavior.

But importing not at the top level will allow to raise an error in runtime, we do not need to call k2_import_guard():

if backend == "k2":
    if criterion_type == "ml":
        from nemo.collections.asr.parts.k2.ml_loss import MLLoss as K2Loss  # maybe error here
    elif criterion_type == "map":
        from nemo.collections.asr.parts.k2.map_loss import MAPLoss as K2Loss  # or here

This doesn't change the previous behavior: in modules k2_import_guard() call was used at the top level, so incorrect k2 version resulted in immediate crash before importing any defined class.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You can set __K2_MINIMUM_MINOR_VERSION = 111 and see that the behavior didn't change and everything works.

Signed-off-by: Vladimir Bataev <[email protected]>
Copy link
Collaborator

@titu1994 titu1994 left a comment

Choose a reason for hiding this comment

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

Approved for now, but K2 should switch to dynamic Numba style runtime check rather than static file import check.

@artbataev artbataev merged commit c640325 into NVIDIA:main Feb 13, 2023
@artbataev artbataev deleted the refactor_k2_import_guard branch February 13, 2023 20:22
titu1994 pushed a commit to titu1994/NeMo that referenced this pull request Mar 24, 2023
* Use module-based k2 import guard

Signed-off-by: Vladimir Bataev <[email protected]>

---------

Signed-off-by: Vladimir Bataev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ASR core Changes to NeMo Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants