Skip to content

refactor ChatCompletionsRequest #375

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

Conversation

declark1
Copy link
Collaborator

@declark1 declark1 commented Apr 16, 2025

NOTE: I want to test this a bit more and add a deserialization unit test, but opening this for review now. This seems to be the only approach that I can see working for this.

Changes:

  • Refactors ChatCompletionRequest to a subset of fields used by the orchestrator (detectors, stream, model, messages) and inner: Map<String, Value> containing the full request
    • Uses #[serde(try_from = "Map<String, Value>")] for deserialization, with the validation and extraction of the fields noted above happening in the TryFrom<Map<String, Value>> for ChatCompletionsRequest implementation
    • Has custom Serialize implementation, which just serializes the inner request
  • Updated chat completions detection integration tests to use the json!() macro instead of ChatCompletionsRequest as the latter now needs to be built via deserialization rather than directly
  • Dropped orchestrator_validation_error integration test scenario for unknown fields and these can no longer be validated. We only validate fields used by the service, with full validation now delegated to the downstream server.

Closes #367

@declark1 declark1 force-pushed the refactor_chat_completions_request branch from a79ac02 to 884fd34 Compare April 16, 2025 22:17
@declark1 declark1 marked this pull request as ready for review April 16, 2025 23:09
@declark1 declark1 requested a review from mdevino April 16, 2025 23:09
Copy link
Collaborator

@mdevino mdevino left a comment

Choose a reason for hiding this comment

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

Left a few nits. Looks like there are a couple of conflicts with the current code base due to recent merges.

@declark1 declark1 force-pushed the refactor_chat_completions_request branch from aec4fd3 to dfed34f Compare April 18, 2025 00:03
Signed-off-by: declark1 <[email protected]>
@declark1
Copy link
Collaborator Author

declark1 commented Apr 18, 2025

Added unit test, addressed initial review comments, rebased to bring in new tests, and updated the new validation test scenarios accordingly to use json!(). Now ready for final review.

Copy link
Collaborator

@mdevino mdevino left a comment

Choose a reason for hiding this comment

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

LGTM! Left a few nit comments.

declark1 and others added 13 commits April 18, 2025 06:42
Co-authored-by: Mateus Devino <[email protected]>
Signed-off-by: Dan Clark <[email protected]>
Co-authored-by: Mateus Devino <[email protected]>
Signed-off-by: Dan Clark <[email protected]>
Co-authored-by: Mateus Devino <[email protected]>
Signed-off-by: Dan Clark <[email protected]>
Co-authored-by: Mateus Devino <[email protected]>
Signed-off-by: Dan Clark <[email protected]>
Co-authored-by: Mateus Devino <[email protected]>
Signed-off-by: Dan Clark <[email protected]>
Co-authored-by: Mateus Devino <[email protected]>
Signed-off-by: Dan Clark <[email protected]>
Co-authored-by: Mateus Devino <[email protected]>
Signed-off-by: Dan Clark <[email protected]>
Co-authored-by: Mateus Devino <[email protected]>
Signed-off-by: Dan Clark <[email protected]>
Co-authored-by: Mateus Devino <[email protected]>
Signed-off-by: Dan Clark <[email protected]>
Co-authored-by: Mateus Devino <[email protected]>
Signed-off-by: Dan Clark <[email protected]>
Co-authored-by: Mateus Devino <[email protected]>
Signed-off-by: Dan Clark <[email protected]>
Co-authored-by: Mateus Devino <[email protected]>
Signed-off-by: Dan Clark <[email protected]>
Co-authored-by: Mateus Devino <[email protected]>
Signed-off-by: Dan Clark <[email protected]>
declark1 and others added 2 commits April 18, 2025 06:45
Co-authored-by: Mateus Devino <[email protected]>
Signed-off-by: Dan Clark <[email protected]>
Co-authored-by: Mateus Devino <[email protected]>
Signed-off-by: Dan Clark <[email protected]>
@declark1 declark1 requested a review from evaline-ju April 18, 2025 16:57
Copy link
Collaborator

@evaline-ju evaline-ju left a comment

Choose a reason for hiding this comment

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

LGTM!

@declark1 declark1 merged commit 6bc045f into foundation-model-stack:main Apr 18, 2025
2 checks passed
@declark1 declark1 deleted the refactor_chat_completions_request branch April 18, 2025 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Account for additional chat completion parameters for /chat/completions-detection
4 participants