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: openai chat message dump logic #7849

Merged
merged 6 commits into from
Dec 16, 2024

Conversation

dongs0104
Copy link
Contributor

What does the PR do?

I found an issue with the OpenAI message format where user messages that are not strings are automatically converted to strings by the code, resulting in a [ChatCompletionResponseMessageContentPart(text="~~~")] string conversion problem. I have fixed this issue.

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:

#7561

Where should the reviewer start?

@rmccorm4

Test plan:

Caveats:

Background

To fix the issue of ChatCompletionResponseMessageContentPart being mixed with the output

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

  • closes GitHub issue: #xxx

@nnshah1 nnshah1 requested a review from rmccorm4 December 3, 2024 16:52
@rmccorm4
Copy link
Contributor

rmccorm4 commented Dec 4, 2024

Hi @dongs0104, thanks for contributing this fix!

  1. Have you emailed a signed CLA as described here: https://github.com/triton-inference-server/server/blob/main/CONTRIBUTING.md#contributor-license-agreement-cla?
  2. Can you share an example request/payload that would produce this string conversion problem before your fix so we can add a test case? If you'd like to add as simple test case, you can refer to here: https://github.com/triton-inference-server/server/blob/main/python/openai/tests/test_chat_completions.py.

@rmccorm4 rmccorm4 self-assigned this Dec 4, 2024
@dongs0104
Copy link
Contributor Author

@rmccorm4 i signed it and add test case

@rmccorm4
Copy link
Contributor

rmccorm4 commented Dec 14, 2024

Started pipeline: 21452750

@rmccorm4
Copy link
Contributor

rmccorm4 commented Dec 14, 2024

Hi @dongs0104,

Have you tested this is passing locally?

I get a schema validation error on your new test case (return code 422) because content is expected to be a string or List[dict], but you're passing a dict directly:

content: Union[str, List[ChatCompletionRequestMessageContentPart]] = Field(

Your test case:

    def test_chat_completions_user_prompt_dict(self, client, model: str):
        messages = [
            {
                "role": "user",
                "content": {"type": "text", "text": "What is machine learning?"},
            }
        ]

Expected Schema: (note the list [ ])

    def test_chat_completions_user_prompt_dict(self, client, model: str):
        # Pass a List of Dicts for content to expose the 'type' field
        messages = [
            {
                "role": "user",
                "content": [{"type": "text", "text": "What is machine learning?"}],
            }
        ]

I think this should get the test to pass.

@dongs0104
Copy link
Contributor Author

@rmccorm4 i tested it after your comment thanks

@nv-kmcgill53
Copy link
Contributor

CLA has been accepted. This PR can be merged at Triton's discretion.

@rmccorm4 rmccorm4 merged commit 2ebd762 into triton-inference-server:main Dec 16, 2024
3 checks passed
@rmccorm4
Copy link
Contributor

Thanks for your contribution @dongs0104!

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