-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Add session customization params #2196
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
base: main
Are you sure you want to change the base?
Conversation
…etrieval from sessions and include new test.
seratch
left a comment
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.
Thanks for sending this pull request! Adding the limit option is reasonable, but we'd like to make the new option to be open to future enhancement. So can you create new SessionSettings class (in the same way with ModelSettings) and move the liimt parameter as the first and only property in it?
…n, including default item retrieval limits.
…ed session management
…eval limits and add corresponding tests
…em retrieval limits and add corresponding tests
…al limits and add corresponding tests
…trieval limits and add corresponding tests
…trieval limits and add corresponding tests
…its and add corresponding tests
…or item retrieval limits and add corresponding tests
…pose the conversation ID with lazy initialization check.
Thanks for the feedback! I've refactored the implementation, following the same pattern as Changes made:
I believe this is extensible for future properties. For example, I'd like to add Let me know if it needs any adjustments |
…nd SessionSettings objects.
|
Can you also resolve the conflicts? |
|
I haven't checked what's wrong here, but the tests are still failing |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e590fcb5d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
I forget to make it into Draft as I was planning to validate everything afterwards. It should work now |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: decd19f31d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
feat: Add
session_limitparameter to control conversation history retrieval from sessions and include new test.