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

Simplify Python impl for KafkaSourceStage #300

Merged
33 commits merged into from
Nov 15, 2022

Conversation

dagardner-nv
Copy link
Contributor

@dagardner-nv dagardner-nv commented Jul 22, 2022

Current impl is overly complicated making use of undocumented methods.
Removes dependency on cudf_kafka.

Fixes #294

@dagardner-nv dagardner-nv added non-breaking Non-breaking change improvement Improvement to existing functionality 2 - In Progress labels Jul 22, 2022
@dagardner-nv dagardner-nv requested a review from a team as a code owner July 22, 2022 15:32
@BartleyR BartleyR added the DO NOT MERGE PR should not be merged; see PR for details label Sep 29, 2022
@dagardner-nv dagardner-nv changed the base branch from branch-22.08 to branch-22.11 October 3, 2022 14:58
@dagardner-nv dagardner-nv added 3 - Ready for Review and removed DO NOT MERGE PR should not be merged; see PR for details 2 - In Progress labels Oct 21, 2022
@dagardner-nv
Copy link
Contributor Author

/rerun tests

@dagardner-nv dagardner-nv changed the title Draft: Simplify Python impl for KafkaSourceStage Simplify Python impl for KafkaSourceStage Oct 24, 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.

Are there any tests that compare the C++ and Python implementations?

@dagardner-nv
Copy link
Contributor Author

Are there any tests that compare the C++ and Python implementations?

I just added one, its a bit tricky as commits perform asynchronously. I had to add a new bool constructor argument to enable blocking commits.

Even then the C++ impl commits just after calling on_next

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.

Looks good. Lets get branch-22.11 merged so we can get CI passing again.

morpheus/stages/input/kafka_source_stage.py Outdated Show resolved Hide resolved
@mdemoret-nv
Copy link
Contributor

@gpucibot merge

@ghost ghost merged commit e580adc into nv-morpheus:branch-22.11 Nov 15, 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
improvement Improvement to existing functionality non-breaking Non-breaking change
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[BUG] KafkaSourceStage Python & C++ source impls commit differently
3 participants