-
Notifications
You must be signed in to change notification settings - Fork 6
add support for model parameters passing #98
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -457,6 +457,26 @@ taskflow: | |||||
|
|
||||||
| The model version can then be updated by changing `gpt_latest` in the `model_config` file and applied across all taskflows that use the config. | ||||||
|
|
||||||
| In addition, model specific parameters can be provided via `model_config`. To do so, define a `model_settings` section in the `model_config` file. This section has to be a dictionary with the model names as keys: | ||||||
|
|
||||||
| ```yaml | ||||||
| model_settings: | ||||||
| gpt_latest: | ||||||
| temperature: 1 | ||||||
| reasoning: | ||||||
| effort: high | ||||||
| ``` | ||||||
|
|
||||||
| You do not need to set parameters for all models defined in the `models` section. When parameters is not set for a model, they'll fall back to the default value. However, all the settings in this section must belong to one of the models specified in the `models` section, otherwise an error will raise: | ||||||
|
||||||
| You do not need to set parameters for all models defined in the `models` section. When parameters is not set for a model, they'll fall back to the default value. However, all the settings in this section must belong to one of the models specified in the `models` section, otherwise an error will raise: | |
| You do not need to set parameters for all models defined in the `models` section. When parameters is not set for a model, they'll fall back to the default value. However, all the settings in this section must belong to one of the models specified in the `models` section, otherwise an error will be raised: |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -91,6 +91,18 @@ Tasks can optionally specify which Model to use on the configured inference endp | |||||
|
|
||||||
| Note that model identifiers may differ between OpenAI compatible endpoint providers, make sure you change your model identifier accordingly when switching providers. If not specified, a default LLM model (`gpt-4o`) is used. | ||||||
|
|
||||||
| Parameters to the model can also be specified in the task using the `model_settings` section: | ||||||
|
|
||||||
| ```yaml | ||||||
| model: gpt-5-mini | ||||||
|
||||||
| model: gpt-5-mini | |
| model: gpt-4o-mini |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -107,7 +107,7 @@ | |||||||||||||
| exclude_from_context: bool = False, | ||||||||||||||
| max_turns: int = DEFAULT_MAX_TURNS, | ||||||||||||||
| model: str = DEFAULT_MODEL, | ||||||||||||||
| model_settings: ModelSettings | None = None, | ||||||||||||||
| model_par: dict = {}, | ||||||||||||||
|
||||||||||||||
| run_hooks: TaskRunHooks | None = None, | ||||||||||||||
| agent_hooks: TaskAgentHooks | None = None): | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -130,10 +130,11 @@ | |||||||||||||
|
|
||||||||||||||
| # https://openai.github.io/openai-agents-python/ref/model_settings/ | ||||||||||||||
| parallel_tool_calls = True if os.getenv('MODEL_PARALLEL_TOOL_CALLS') else False | ||||||||||||||
| model_settings = ModelSettings( | ||||||||||||||
| temperature=os.getenv('MODEL_TEMP', default=0.0), | ||||||||||||||
| tool_choice=('auto' if toolboxes else None), | ||||||||||||||
| parallel_tool_calls=(parallel_tool_calls if toolboxes else None)) | ||||||||||||||
| model_params = {'temperature' : os.getenv('MODEL_TEMP', default = 0.0), | ||||||||||||||
|
||||||||||||||
| model_params = {'temperature' : os.getenv('MODEL_TEMP', default = 0.0), | |
| model_params = {'temperature' : os.getenv('MODEL_TEMP', default=0.0), |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The os.getenv('MODEL_TEMP', default=0.0) call returns a string, not a float. Environment variables are always strings, so this will pass "0.0" (a string) to ModelSettings instead of the numeric value 0.0. This should be float(os.getenv('MODEL_TEMP', default='0.0')) or use default=0.0 only after converting the environment variable value to a float.
| model_params = {'temperature' : os.getenv('MODEL_TEMP', default = 0.0), | |
| model_params = {'temperature' : float(os.getenv('MODEL_TEMP', default='0.0')), |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent spacing around the assignment operator. The code uses 'temperature' : os.getenv(...) with spaces around the colon, which is not standard Python style. According to PEP 8, dictionary keys should not have spaces around the colon. This should be 'temperature': os.getenv(...) (and similarly for the other keys).
| model_params = {'temperature' : os.getenv('MODEL_TEMP', default = 0.0), | |
| 'tool_choice' : ('auto' if toolboxes else None), | |
| 'parallel_tool_calls' : (parallel_tool_calls if toolboxes else None)} | |
| model_params = {'temperature': os.getenv('MODEL_TEMP', default = 0.0), | |
| 'tool_choice': ('auto' if toolboxes else None), | |
| 'parallel_tool_calls': (parallel_tool_calls if toolboxes else None)} |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation logic if not set(models_params.keys()).difference(model_keys).issubset(set([])) is unnecessarily complex. The condition .issubset(set([])) is equivalent to checking if the set is empty. This should be simplified to if set(models_params.keys()) - set(model_keys): or if not set(models_params.keys()).issubset(model_keys): for better readability.
| if not set(models_params.keys()).difference(model_keys).issubset(set([])): | |
| if not set(models_params.keys()).issubset(model_keys): |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message says "Settings section...contains models that are not in the model section" but should say "models that are not in the models section" (with an 's' on 'models'). Additionally, this error message could be more helpful by indicating which models are problematic. Consider: f"Settings section of model_config file {model_config} contains models that are not defined in the models section: {', '.join(set(models_params.keys()) - set(model_keys))}"
| if not set(models_params.keys()).difference(model_keys).issubset(set([])): | |
| raise ValueError(f"Settings section of model_config file {model_config} contains models that are not in the model section") | |
| problematic_models = set(models_params.keys()) - set(model_keys) | |
| raise ValueError( | |
| f"Settings section of model_config file {model_config} contains models that are not defined in the models section: {', '.join(problematic_models)}" | |
| ) |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after comma in for k,v in models_params.items():. According to PEP 8, there should be a space after the comma: for k, v in models_params.items():.
| for k,v in models_params.items(): | |
| for k, v in models_params.items(): |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Missing space after comma in the loop. Should be for k, v in instead of for k,v in according to PEP 8 style guidelines.
| for k,v in models_params.items(): | |
| for k, v in models_params.items(): |
m-y-mo marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Check warning
Code scanning / CodeQL
Variable defined multiple times Warning
redefined
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assignment to 'name' is unnecessary as it is redefined before this value is used.
| name = task_body.get('name', 'taskflow') # placeholder, not used yet |
Uh oh!
There was an error while loading. Please reload this page.