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

Fixes Timestamp Nodes When Running with --debug #145

Merged
4 commits merged into from
Jun 8, 2022

Conversation

mdemoret-nv
Copy link
Contributor

If the user runs with --debug, then any stage that derives from MultiMessageStage, will insert columns with the message timestamp for when it was finished processing. With some of the recent changes to Neo, we disabled broadcast nodes. However, the timestamp code didnt get updated.

This PR fixes a few things:

  1. It fixes the timestamp node to return the input object instead of None
  2. It updates all nodes to require overriding supports_cpp_node
    1. This is necessary to ensure _build_cpp_node() returns the correct thing
  3. It only inserts timestamp nodes when C++ is disabled (we should have a C++ version instead).
    1. This uses _build_cpp_node() to determine whether C++ is being used for this particular stage
  4. It updates all nodes to use _build_cpp_node() instead of CppConfig.get_should_use_cpp()

Fixes #143

@mdemoret-nv mdemoret-nv added bug Something isn't working breaking Breaking change 3 - Ready for Review labels Jun 7, 2022
@mdemoret-nv mdemoret-nv requested a review from dagardner-nv June 7, 2022 18:17
@mdemoret-nv mdemoret-nv requested a review from a team as a code owner June 7, 2022 18:17
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 suspect that the switch away from using CppConfig directly left some unused imports that the check stage is going to fail on.

@dagardner-nv dagardner-nv mentioned this pull request Jun 8, 2022
@mdemoret-nv
Copy link
Contributor Author

@gpucibot merge

@ghost ghost merged commit 55edc6a into nv-morpheus:branch-22.06 Jun 8, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] nlp_si_detection example failing
2 participants