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

fix: Fix bug when targeting the TRT-LLM backend ensemble #7700

Merged

Conversation

blongnv
Copy link
Contributor

@blongnv blongnv commented Oct 14, 2024

What does the PR do?

If a user follows the example code and tries to use the ensemble instead of tensorrt_llm_bls, they will get this error:

curl -s http://localhost:9000/v1/chat/completions -H 'Content-Type: application/json' -d '{"model": "ensemble", "messages": [{"role": "user", "content": "Say this is a test!"}]}' | jq
INFO:     127.0.0.1:40064 - "POST /v1/chat/completions HTTP/1.1" 400 Bad Request
{
  "detail": "Unknown backend"
}

After the PR fix, the response will be correct:

curl -s http://localhost:9000/v1/chat/completions -H 'Content-Type: application/json' -d '{"model": "ensemble", "messages": [{"role": "user", "content": "Say this is a test!"}]}' | jq
INFO:     127.0.0.1:47946 - "POST /v1/chat/completions HTTP/1.1" 200 OK
{
  "id": "cmpl-fefc9e2a-8a74-11ef-8e2a-0242c0a80102",
  "choices": [
    {
      "finish_reason": "stop",
      "index": 0,
      "message": {
        "content": "This is a test!",
        "tool_calls": null,
        "role": "assistant",
        "function_call": null
      },
      "logprobs": null
    }
  ],
  "created": 1728942057,
  "model": "ensemble",
  "system_fingerprint": null,
  "object": "chat.completion",
  "usage": null
}

Checklist

  • I have read the Contribution guidelines and signed the Contributor License
    Agreement
  • 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.
  • I ran pre-commit locally (pre-commit install, pre-commit run --all)
  • 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:

Where should the reviewer start?

Test plan:

Caveats:

Background

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

  • closes GitHub issue: #xxx

@rmccorm4 rmccorm4 changed the title Fix bug when targeting the TRT-LLM backend ensemble fix: Fix bug when targeting the TRT-LLM backend ensemble Oct 14, 2024
@rmccorm4 rmccorm4 added the PR: fix A bug fix label Oct 14, 2024
@rmccorm4
Copy link
Contributor

ref: DLIS-7451 to follow-up with a test for ensemble path to maintain support and catch regressions

@rmccorm4
Copy link
Contributor

Pipeline: 19345468 ✅

rmccorm4
rmccorm4 previously approved these changes Oct 14, 2024
@@ -250,6 +250,9 @@ def _get_model_metadata(self) -> Dict[str, TritonModelMetadata]:
for name, _ in self.server.models().keys():
model = self.server.model(name)
backend = model.config()["backend"]
if not backend:
# Check platform field as a backup, this will support 'ensemble' models
backend = model.config()["platform"]
Copy link
Contributor

Choose a reason for hiding this comment

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

@rmccorm4 should we check against a list of supported platform types here to rule out any accidents

Copy link
Contributor

@rmccorm4 rmccorm4 Oct 15, 2024

Choose a reason for hiding this comment

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

Sure, we can be strict here just in case.

There's currently no use case I'm aware of where the platform would be used to specify a backend (ex: platform: tensorrt_plan) that doesn't have a backend equivalent (other than ensemble), and we generally encourage use of backend for this purpose for pretty much everything other than ensembles - so I think it's fine to be strict for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

@blongnv could you modify it to be something like this?

# Explicitly handle ensembles to avoid any runtime validation errors
if not backend and model.config()["platform"] == "ensemble":
  backend = "ensemble"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented, please check

@rmccorm4 rmccorm4 merged commit 479486f into triton-inference-server:main Oct 16, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: fix A bug fix
Development

Successfully merging this pull request may close these issues.

3 participants