Skip to content

test: Add tests cancelling BLS decoupled request in Python backend #8097

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

Merged
merged 6 commits into from
Apr 8, 2025

Conversation

richardhuo-nv
Copy link
Contributor

@richardhuo-nv richardhuo-nv commented Mar 24, 2025

What does the PR do?

Add tests cancelling BLS decoupled request in Python backend.

Checklist

  • PR title reflects the change and is of format <commit_type>: <Title>
  • Changes are described in the pull request.
  • Related issues are referenced.
  • Populated github labels field
  • Added test plan and verified test passes.
  • Verified that the PR passes existing CI.
  • Verified copyright is correct on all changed files.
  • Added succinct git squash message before merging ref.
  • All template sections are filled out.
  • Optional: Additional screenshots for behavior/output changes with before/after.

Commit Type:

Check the conventional commit type
box here and add the label to the github PR.

  • build
  • ci
  • docs
  • feat
  • fix
  • perf
  • refactor
  • revert
  • style
  • test

Related PRs:

triton-inference-server/python_backend#398

Where should the reviewer start?

Check the response_sender_until_cancelled python decoupled model added and then check the two added testing models.
The tests are based on the two added testing models sending BLS requests to the response_sender_until_cancelled python decoupled model.

Test plan:

  • CI Pipeline ID: 25879687

Caveats:

NA

Background

NA

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

https://jirasw.nvidia.com/browse/DLIS-7831

# OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
import asyncio

Check notice

Code scanning / CodeQL

Unused import Note

Import of 'asyncio' is not used.

Copilot Autofix

AI 3 months ago

To fix the problem, we need to remove the unused import statement for the asyncio module. This will eliminate the unnecessary dependency and make the code cleaner and easier to read. The change should be made in the file qa/L0_backend_python/decoupled/models/decoupled_bls_async_cancel/1/model.py by deleting the line that imports asyncio.

Suggested changeset 1
qa/L0_backend_python/decoupled/models/decoupled_bls_async_cancel/1/model.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/qa/L0_backend_python/decoupled/models/decoupled_bls_async_cancel/1/model.py b/qa/L0_backend_python/decoupled/models/decoupled_bls_async_cancel/1/model.py
--- a/qa/L0_backend_python/decoupled/models/decoupled_bls_async_cancel/1/model.py
+++ b/qa/L0_backend_python/decoupled/models/decoupled_bls_async_cancel/1/model.py
@@ -25,3 +25,2 @@
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-import asyncio
 
EOF
@@ -25,3 +25,2 @@
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
import asyncio

Copilot is powered by AI and may make mistakes. Always verify output.
@richardhuo-nv richardhuo-nv committed this autofix suggestion 3 months ago.
@kthui
Copy link
Contributor

kthui commented Mar 24, 2025

Nice work adding tests for both async and regular decoupled BLS request cancellation!

Request cancellation is best effort in Triton, meaning the cancellation only "happens" if the backend implements it. Can we add two new tests, in addition to the two tests, for checking the behavior of BLS cancellation when the inner model does not honor request cancellation, which the inner model will return all the responses and final flag normally despite cancellation is requested?

Edit: Can we also add a small test checking what will happen if the model is trying to cancel a completed/finialed BLS request?

@richardhuo-nv
Copy link
Contributor Author

Nice work adding tests for both async and regular decoupled BLS request cancellation!

Request cancellation is best effort in Triton, meaning the cancellation only "happens" if the backend implements it. Can we add two new tests, in addition to the two tests, for checking the behavior of BLS cancellation when the inner model does not honor request cancellation, which the inner model will return all the responses and final flag normally despite cancellation is requested?

Edit: Can we also add a small test checking what will happen if the model is trying to cancel a completed/finialed BLS request?

Thanks! Tests added.

# OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
import asyncio

Check notice

Code scanning / CodeQL

Unused import Note

Import of 'asyncio' is not used.

Copilot Autofix

AI 3 months ago

To fix the problem, we need to remove the unused import statement for the asyncio module. This will clean up the code and remove the unnecessary dependency, making the code easier to read and maintain.

  • Locate the import statement for asyncio on line 26.
  • Remove the line import asyncio from the file qa/L0_backend_python/decoupled/models/decoupled_bls_cancel_after_complete/1/model.py.
Suggested changeset 1
qa/L0_backend_python/decoupled/models/decoupled_bls_cancel_after_complete/1/model.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/qa/L0_backend_python/decoupled/models/decoupled_bls_cancel_after_complete/1/model.py b/qa/L0_backend_python/decoupled/models/decoupled_bls_cancel_after_complete/1/model.py
--- a/qa/L0_backend_python/decoupled/models/decoupled_bls_cancel_after_complete/1/model.py
+++ b/qa/L0_backend_python/decoupled/models/decoupled_bls_cancel_after_complete/1/model.py
@@ -25,4 +25,2 @@
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-import asyncio
-
 import numpy as np
EOF
@@ -25,4 +25,2 @@
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
import asyncio

import numpy as np
Copilot is powered by AI and may make mistakes. Always verify output.
@richardhuo-nv richardhuo-nv committed this autofix suggestion 3 months ago.
@richardhuo-nv richardhuo-nv requested a review from yinggeh March 31, 2025 16:55
@yinggeh
Copy link
Contributor

yinggeh commented Apr 7, 2025

Please avoid force pushing commits as it makes it harder for reviewers to track your changes.

@richardhuo-nv
Copy link
Contributor Author

richardhuo-nv commented Apr 7, 2025

Please avoid force pushing commits as it makes it harder for reviewers to track your changes.

Sorry about that, I will not do it next time.

All changes from the last review are in the second commit of this PR, you can check the commit history and click the second commit and find all the changes. I had a habit of squash changes into each one commit for each round of review, but next time I will not squash commits. Thanks!

Copy link
Contributor

@yinggeh yinggeh left a comment

Choose a reason for hiding this comment

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

Nice work! Make sure new tests pass the CI pipeline.

@richardhuo-nv richardhuo-nv merged commit 6c6df11 into main Apr 8, 2025
3 checks passed
@richardhuo-nv richardhuo-nv deleted the rihuo/bls-cancel branch April 8, 2025 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants