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

test: Allow ensemble to create the final response even if some of the outputs are not created #7701

Merged
merged 1 commit into from
Oct 16, 2024

Conversation

kthui
Copy link
Contributor

@kthui kthui commented Oct 15, 2024

What does the PR do?

Add test when the composing models of an ensemble do not provide all outputs on the model configuration. The inference should complete normally and return all outputs that are provided.

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/core#399

Where should the reviewer start?

N/A

Test plan:

N/A

  • CI Pipeline ID: 19348789

Caveats:

N/A

Background

N/A

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

N/A

@kthui kthui force-pushed the jacky-ensemble-output branch from 0706230 to d40d8ff Compare October 15, 2024 00:14
@kthui kthui changed the title test: Add test for partial ensemble outputs test: Allow ensemble to create the final response even if some of the outputs are not created Oct 15, 2024
@kthui kthui marked this pull request as ready for review October 15, 2024 19:21
@kthui kthui requested review from nnshah1, rmccorm4 and GuanLuo October 15, 2024 19:22
@kthui kthui merged commit d99a877 into main Oct 16, 2024
3 checks passed
@kthui kthui deleted the jacky-ensemble-output branch October 16, 2024 16:41
@@ -177,6 +178,41 @@ def callback(start_time, result, error):
if expected_flags != response_flags:
self.assertTrue(False, "unexpeced sequence flags mismatch error")

def test_ensemble_partial_add_sub(self):
Copy link
Contributor

@rmccorm4 rmccorm4 Oct 16, 2024

Choose a reason for hiding this comment

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

I think we should have more testing in general to programatically track expected behavior of certain edge cases, such as:

  1. What if a composing model doesn't produce the outputs necessary to feed inputs of the next step?
  2. In an ensemble with outputs from 2 parallel steps, what if 1 step has outputs and the other step has no outputs?
  3. In an ensemble with outputs from 2 parallel steps, what if 1 step has outputs and the other step encounters an error? Is it considered OK to return the completed steps outputs, or is it an overall error?
  4. What if an ensemble doesn't produce any of its outputs?
  5. What if

These aren't immediately clear to me from scanning the code what would happen, so I think it would be good to enforce expected behavior via testing, and be able to catch if the behavior subtlely changes from future ensemble code changes

Copy link
Contributor

Choose a reason for hiding this comment

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

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