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

Extraction of common scripts and utilities + cleanup and imrpovements #1

Merged
61 commits merged into from
Jan 6, 2023

Conversation

drobison00
Copy link
Contributor

@drobison00 drobison00 commented Dec 2, 2022

Consolidates common functionality for cmake, scripts, conda, debugging, etc.. from nv-morpheus/MRC and nv-morpheus/Morpheus.

CMake tools can be added at various levels of completeness, and should satisfy their own pre-requisites at each level.

include(cmake/morpheus_utils/load) : Enables all morpheus_utils_xxx library calls
include(cmake/morpheus_utils/.../register_api): Enables morpheus_utils_xx library calls for the given sub-library

cmake/morpheus_utils/
environment_config - contains items related to build environment, compiler setup, cmake configuration, caching, rapids_cpm, etc...
general - General helpers, tools, debugging code, etc..
grpc - GRPC helpers, scripts, and definitions
package_config - Wrapper functions for pulling, loading, and building various libraries that nv-morpheus repositories use.
package_search - FindXXX files
python - Python specific helper tools, pybind11 utilities, macros, functions, and utilities

cmake/conda
Nothing currently; while both repos duplicate code, there are specific and subtle differences between each. Consolidating is better left to its own PR.

cmake/docker
Similar to conda, much of the duplicated code doesn't have a clear path to generalizing/merging. Consolidation here is also better left to be its own PR.

cmake/misc
Items that were either unused in either repo but we want to retain, or artifacts that had no clear home.

cmake/scripts
Common scripts for bash, python, etc.. that can be shared across repositories.

@drobison00 drobison00 added the enhancement Additional functionality added to an existing feature label Dec 2, 2022
@drobison00 drobison00 requested a review from a team as a code owner December 2, 2022 22:04
@drobison00 drobison00 added non-breaking Non-breaking change 3 - Ready for Review PR/Issue is complete and ready for review by team improvement Improvement to existing functionality labels Dec 13, 2022
@drobison00 drobison00 changed the title [DRAFT, DO_NOT_MERGE] Initial extraction of common scripts and utilities + cleanup and imrpovements Extraction of common scripts and utilities + cleanup and imrpovements Dec 13, 2022
Copy link
Contributor

@mdemoret-nv mdemoret-nv left a comment

Choose a reason for hiding this comment

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

Some general comments:

  1. I think there is a lot in the ci folder which could be moved to utilities. Many of the ci/scripts files are nearly the same. We could include that in this PR or separate into a second PR
  2. The files in the conda and docker directories seem to be off. Many of the docker related files should be moved to the docker directory
  3. We can get rid of the files in misc
  4. We should use this as a good time to enforce consistent file names. i.e. all lower camel case.
  5. Since this will be used by multiple repos, we should ensure that there are no unintended side effects. For example, if you do include(cmake/morpheus_utils/load) it checks for pybind11, python and skbuild. These arent necessary for all repos but will be required now.
    1. We should try to enforce that loading this repo doesnt have any effects until specific functions are called. i.e. the register_api files just create macros/functions but dont run anything.

Copy link
Contributor

@mdemoret-nv mdemoret-nv left a comment

Choose a reason for hiding this comment

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

Just a couple of changes and we should be good.

@drobison00
Copy link
Contributor Author

/merge

@ghost ghost merged commit a83eb0f into branch-23.01 Jan 6, 2023
@mdemoret-nv mdemoret-nv deleted the devin-issue-225-87 branch January 27, 2023 16:32
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review PR/Issue is complete and ready for review by team enhancement Additional functionality added to an existing feature improvement Improvement to existing functionality non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants