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

Eliminate redundant code blocks in modules and stages #1123

Merged
merged 26 commits into from
Sep 6, 2023
Merged

Eliminate redundant code blocks in modules and stages #1123

merged 26 commits into from
Sep 6, 2023

Conversation

bsuryadevara
Copy link
Contributor

@bsuryadevara bsuryadevara commented Aug 8, 2023 •
edited
Loading

Eliminated redundant code blocks in both modules and stages, introducing controllers to enhance maintainability, and subsequently updated tests to align with these changes.

  • file_to_df
  • filter_detections
  • mlflow_model_writer
  • serializer
  • write_to_file

Fixed preserve columns property issue.
closes #965 #1074

@bsuryadevara bsuryadevara added enhancement Additional functionality added to an existing feature non-breaking Non-breaking change 3 - Ready for Review labels Aug 8, 2023
@bsuryadevara bsuryadevara self-assigned this Aug 8, 2023
@bsuryadevara bsuryadevara linked an issue Aug 8, 2023 that may be closed by this pull request
[FEA]: Identify and Remove Duplicate Code Blocks in Modules #965
Closed
7 tasks
@bsuryadevara bsuryadevara added improvement Improvement to existing functionality feature request New feature or request and removed enhancement Additional functionality added to an existing feature feature request New feature or request labels Aug 8, 2023
@bsuryadevara bsuryadevara changed the title Eliminated redundant code in modules and stages Eliminate redundant code blocks in modules and stages Aug 9, 2023
@bsuryadevara bsuryadevara marked this pull request as ready for review August 9, 2023 19:20
@bsuryadevara bsuryadevara requested a review from a team as a code owner August 9, 2023 19:20
cwharris
cwharris approved these changes Aug 10, 2023
View reviewed changes
Copy link
Contributor

@cwharris cwharris left a comment

Choose a reason for hiding this comment

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

LGTM. Glad to see this consolidation!

@mdemoret-nv mdemoret-nv self-requested a review August 17, 2023 02:20
@bsuryadevara
Copy link
Contributor Author

Changed to in progress. Fixing broken tests

@bsuryadevara bsuryadevara linked an issue Aug 22, 2023 that may be closed by this pull request
[BUG]: Fix DataFrameInputSchema.preserve_columns #1074
Closed
2 tasks
mdemoret-nv
mdemoret-nv requested changes Aug 31, 2023
View reviewed changes
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.

A few general items:

  • Move the controllers out of the utils directory and to their own directory morpheus/controllers
  • Can you verify that there are no other modules that do not use a controller?
  • Would it make more sense for some of the stages to just use the module

examples/digital_fingerprinting/production/morpheus/dfp/stages/dfp_file_to_df.py Outdated Show resolved Hide resolved
examples/digital_fingerprinting/production/morpheus/dfp/stages/dfp_file_to_df.py Outdated Show resolved Hide resolved
examples/digital_fingerprinting/production/morpheus/dfp/utils/model_cache.py Outdated Show resolved Hide resolved
examples/digital_fingerprinting/production/morpheus/dfp/utils/model_cache.py Outdated Show resolved Hide resolved
examples/ransomware_detection/common/feature_extractor.py Outdated Show resolved Hide resolved
@copy-pr-bot
Copy link

copy-pr-bot bot commented Aug 31, 2023

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@bsuryadevara
Copy link
Contributor Author

A few general items:

  • Move the controllers out of the utils directory and to their own directory morpheus/controllers
    Done
  • Can you verify that there are no other modules that do not use a controller?
    Verified there aren't any. Moved monitor controller from utils to controllers package
  • Would it make more sense for some of the stages to just use the module
    Yes, definitely. Going forward for any new modules (except modules that has external services like elasticsearch or db's, as it makes harder to test the implementation) we can just put the implementation within the modules itself.

@mdemoret-nv
Copy link
Contributor

/ok to test

mdemoret-nv
mdemoret-nv approved these changes Sep 5, 2023
View reviewed changes
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.

Couple more items. Should be good to merge once the comments are resolved.

examples/ransomware_detection/common/feature_extractor.py Outdated Show resolved Hide resolved
examples/digital_fingerprinting/production/morpheus/dfp/utils/model_cache.py Outdated Show resolved Hide resolved
@mdemoret-nv
Copy link
Contributor

/ok to test

@bsuryadevara
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit be608fd into nv-morpheus:branch-23.11 Sep 6, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cwharris cwharris cwharris approved these changes

@mdemoret-nv mdemoret-nv mdemoret-nv approved these changes

Assignees

@bsuryadevara bsuryadevara

Labels
improvement Improvement to existing functionality non-breaking Non-breaking change
Projects
Morpheus Boards
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG]: Fix DataFrameInputSchema.preserve_columns [FEA]: Identify and Remove Duplicate Code Blocks in Modules
3 participants
@bsuryadevara @mdemoret-nv @cwharris