[Feat] Add presence_penalty and frequency_penalty#316
Conversation
Signed-off-by: Li, Tianmu <tianmu.li@intel.com>
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
There was a problem hiding this comment.
Code Review
This pull request adds presence_penalty and frequency_penalty parameters to the ModelParams schema, configuration templates, and OpenAI adapters. A review comment suggests improving the ColumnFilter implementation in completions_adapter.py by removing metadata.keys() from the optional_columns list to ensure consistency with other adapters and clarify the data flow.
| ColumnFilter( | ||
| required_columns=["input_tokens"], | ||
| optional_columns=["n", "presence_penalty", "frequency_penalty", "stop"] | ||
| + list(metadata.keys()), | ||
| optional_columns=["n", "stop"] + list(metadata.keys()), | ||
| ), |
There was a problem hiding this comment.
For consistency with other adapters (OpenAIAdapter, OpenAIMsgspecAdapter), it would be cleaner to not include metadata.keys() in optional_columns.
The optional_columns in ColumnFilter should ideally list only the columns that are passed through from the dataset. The columns from model_params are added later by AddStaticColumns. The current implementation where dataset columns are kept only to be immediately overwritten is inefficient and can be confusing.
By simplifying this, we make the data flow clearer and align this adapter with the cleaner pattern used elsewhere in the project.
| ColumnFilter( | |
| required_columns=["input_tokens"], | |
| optional_columns=["n", "presence_penalty", "frequency_penalty", "stop"] | |
| + list(metadata.keys()), | |
| optional_columns=["n", "stop"] + list(metadata.keys()), | |
| ), | |
| ColumnFilter( | |
| required_columns=["input_tokens"], | |
| optional_columns=["n", "stop"], | |
| ), |
There was a problem hiding this comment.
Pull request overview
Adds presence_penalty and frequency_penalty as configurable sampling parameters and forwards them through OpenAI-compatible adapters.
Changes:
- Adds the new penalty fields to
ModelParamsand full config templates. - Adds static dataset metadata for these fields in OpenAI chat/msgspec/completions adapters.
- Forwards the new fields into OpenAI chat/completions request payloads.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/inference_endpoint/openai/openai_msgspec_adapter.py |
Adds penalties to static metadata and removes them from passthrough allow-list. |
src/inference_endpoint/openai/openai_adapter.py |
Adds penalties to metadata and request construction. |
src/inference_endpoint/openai/completions_adapter.py |
Adds penalties to completions metadata and keeps request serialization. |
src/inference_endpoint/config/templates/online_template_full.yaml |
Documents new model parameters in the online full template. |
src/inference_endpoint/config/templates/offline_template_full.yaml |
Documents new model parameters in the offline full template. |
src/inference_endpoint/config/templates/concurrency_template_full.yaml |
Documents new model parameters in the concurrency full template. |
src/inference_endpoint/config/schema.py |
Adds the new penalty fields to the model configuration schema. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "presence_penalty": model_params.presence_penalty, | ||
| "frequency_penalty": model_params.frequency_penalty, |
| "presence_penalty": model_params.presence_penalty, | ||
| "frequency_penalty": model_params.frequency_penalty, | ||
| } |
| "presence_penalty": model_params.presence_penalty, | ||
| "frequency_penalty": model_params.frequency_penalty, |
| "repetition_penalty": model_params.repetition_penalty, | ||
| "presence_penalty": model_params.presence_penalty, | ||
| "frequency_penalty": model_params.frequency_penalty, | ||
| } |
What does this PR do?
Add presence_penalty and frequency_penalty to allowed sampling parameters
Type of change
Related issues
Testing
Checklist