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

Cleanup all uses of import morpheus._lib.common #787

Merged

Conversation

mdemoret-nv
Copy link
Contributor

Description

This PR removes all uses of from morpheus._lib.common import XXX and replaces them with from morpheus.common import XXX. Users should not ever import directly from morpheus._lib. Any public facing symbols that are C++ only (and thus exist in morpheus._lib should be re-exported from the matching python module's __init__.py.

The main reason for this is:

  • Avoid errors that can arise from unintentionally using the C++ version of a class instead of a Python version.
  • Eliminate the possibility of circular imports
  • Hide the use of the non-public morpheus._lib namespace

@mdemoret-nv mdemoret-nv added non-breaking Non-breaking change improvement Improvement to existing functionality labels Mar 22, 2023
@mdemoret-nv mdemoret-nv requested a review from a team as a code owner March 22, 2023 16:24
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Contributor

@dagardner-nv dagardner-nv left a comment

Choose a reason for hiding this comment

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

LGTM
I take it the plan is to follow this up with a similar PR for _lib.stages and _lib.messages?

@mdemoret-nv
Copy link
Contributor Author

LGTM I take it the plan is to follow this up with a similar PR for _lib.stages and _lib.messages?

We could, and it might be good to have a CI check to enforce this, but its much less important for those two packages because all of the public symbols are hidden by their python counterparts. i.e. morpheus._lib.messages.MessageMeta has a morpheus.messages.MessageMeta already.

@mdemoret-nv
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit ca66ec0 into nv-morpheus:branch-23.03 Mar 22, 2023
@mdemoret-nv mdemoret-nv deleted the mdd_cleanup-morpheus._lib branch March 22, 2023 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement to existing functionality non-breaking Non-breaking change
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants