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

PyCoro binding code test #1286

Closed

Conversation

cwharris
Copy link
Contributor

@cwharris cwharris commented Oct 17, 2023

Description

Tests the python/cpp coroutine binding code (pycoro) to ensure recursive/nested and parallel python and c++ calls can be interleaved without issue, that Python/C++ -mixed tasks can be cancelled from Python, and that exceptions throw from both C++ and Python can be caught in Python. Also allows PyTaskToCppAwaitable to be constructed with a raw python coroutine as opposed to only asyncio tasks.

Closes #1268

By Submitting this PR I confirm:

  • I am familiar with the Contributing Guidelines.
  • When the PR is ready for review, new or existing tests cover these changes.
  • When the PR is ready for review, the documentation is up to date with these changes.

@cwharris cwharris added non-breaking Non-breaking change improvement Improvement to existing functionality labels Oct 17, 2023
@cwharris cwharris requested a review from a team as a code owner October 17, 2023 23:29
@cwharris cwharris changed the title Fea sherlock tests PyCoro binding code test Oct 18, 2023
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.

This is a great start but I would like to see more edge cases. The issue has the following listed:

  • Throwing exceptions from C++
  • Throwing exceptions from Python
  • Cancelling coroutines from python
  • Abnormal shutdown of the loop
  • Nested layers of python -> C++ -> python -> C++ coroutines

I only see one of those exercised here

@cwharris cwharris requested a review from mdemoret-nv October 18, 2023 21:31
@mdemoret-nv
Copy link
Contributor

@cwharris Can you get the check stage to pass and then we can merge?

@cwharris cwharris self-assigned this Oct 20, 2023
rapids-bot bot pushed a commit to nv-morpheus/MRC that referenced this pull request Oct 25, 2023
Moves pycoro from Morpheus to MRC and incorperates tests from nv-morpheus/Morpheus#1286

Closes nv-morpheus/Morpheus#1268

Authors:
  - Christopher Harris (https://github.com/cwharris)

Approvers:
  - Devin Robison (https://github.com/drobison00)

URL: #409
@cwharris
Copy link
Contributor Author

Closing in favor of nv-morpheus/MRC#409

@cwharris cwharris closed this Oct 26, 2023
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