Skip to content

Conversation

ziqifan617
Copy link
Contributor

@ziqifan617 ziqifan617 commented Feb 4, 2025

What does the PR do?

Add tests for response parameters support for BLS 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#395

Where should the reviewer start?

Start with test_setting_three_element_response_parameters test case first to get a sense on the feature, and then move on to other test cases, and finish with test_setting_response_parameters_decoupled test case.

Test plan:

  • CI Pipeline ID: 23548252

Caveats:

NA

Background

NA

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

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

@ziqifan617 ziqifan617 added the enhancement New feature or request label Feb 4, 2025
@ziqifan617 ziqifan617 added PR: test Adding missing tests or correcting existing test and removed enhancement New feature or request labels Feb 4, 2025
@ziqifan617 ziqifan617 requested a review from kthui February 4, 2025 23:57
Copy link
Contributor

@kthui kthui 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 on your first test case!

Two things I think we can enhance on:

  • A test case when the BLS composing model is also decoupled.
  • The BLS test may reuse the response_paremeters_test.py if it only differs slightly from the new response_parameters_bls_test.py, i.e. model names.

@kthui kthui requested a review from krishung5 February 5, 2025 01:12
Copy link
Contributor

@krishung5 krishung5 left a comment

Choose a reason for hiding this comment

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

Nice testing! I think the test coverage looks good, but would +1 on what Jacky suggested above, which would avoid some code duplications.

kthui
kthui previously approved these changes Feb 6, 2025
Copy link
Contributor

@kthui kthui 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!

@ziqifan617 ziqifan617 force-pushed the ziqif-bls-resp-params branch from 8b2f2de to bc8f539 Compare February 6, 2025 18:57
@ziqifan617 ziqifan617 merged commit fd921c5 into main Feb 6, 2025
3 checks passed
@ziqifan617 ziqifan617 deleted the ziqif-bls-resp-params branch February 6, 2025 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: test Adding missing tests or correcting existing test
Development

Successfully merging this pull request may close these issues.

3 participants