-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Expose the "store" parameter through ModelSettings #357
Conversation
@@ -30,7 +30,7 @@ class ModelSettings: | |||
tool_choice: Literal["auto", "required", "none"] | str | None = None | |||
"""The tool choice to use when calling the model.""" | |||
|
|||
parallel_tool_calls: bool | None = False | |||
parallel_tool_calls: bool | None = None |
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.
@rm-openai this seemed like a typo, since it meant that if you tried to override a different setting at the Runner.run level it would also set this back to False
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.
actually never mind since an or
was used below it's fine. but we should actually not use or
below because "0" is false-y in Python, so setting "temperature=0" at the Runner.run level would fail to override temperature at the agent level
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.
some lint issues and comments inline
src/agents/model_settings.py
Outdated
|
||
new_values = { | ||
k: getattr(override, k) if getattr(override, k) is not None else getattr(self, k) | ||
for k in asdict(self) | ||
} | ||
return ModelSettings(**new_values) |
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.
feels like unnecessarily expensive to do an asdict, which creates a copy. Why not just use a helper function that sets the param iff its not None?
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.
switched to __dataclass_fields__
which does not create a copy, added a test
* upstream/main: (90 commits) Databricks MLflow tracing integration (openai#401) Add Langtrace to `tracing.md` (openai#371) Remove duplicate dependency definition of pynput (openai#367) Hotfix mcp example (openai#365) Expose the "store" parameter through ModelSettings (openai#357) Raise error on more invalid function schemas (openai#356) Tracing screenshot for MCP docs (openai#355) MCP example for SSE (openai#354) v0.0.7 (openai#353) Mark handoff span as errored when multiple handoffs are requested (openai#344) added readme, fixed typo fixing lint issues adding Git MCP server example [5/n] MCP tracing fix(examples): make sure audio playback finishes Remove Jupyter Notebook files from .gitignore linting Fix type ignore comment for agent check in get_all_edges function Refactor visualization functions to improve formatting and streamline edge generation Add start and end nodes to graph visualization and update edge generation ...
Closes #173
This will also set stored completions to True by default, encouraging a best practice.