Skip to content
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

Remove duplicate params & context args #14

Merged
merged 3 commits into from
Dec 3, 2023

Conversation

rileytomasek
Copy link
Contributor

These are both already handled statefully by the ChatModel so we shouldn't be
introducing a second way to accomplish the same thing and more arguments to
support it.

  • Remove duplicate context arg
  • Remove duplicate params argument

Copy link

vercel bot commented Dec 2, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
dexter ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 3, 2023 0:19am

Copy link
Collaborator

@transitive-bullshit transitive-bullshit left a comment

Choose a reason for hiding this comment

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

Ooooh interesting; I wasn't aware of chatModel.addParams, and given how familiar I am / should be with Dexter, if I wasn't aware of it, then most devs viewing this lib wouldn't have been.

But given that addParams mutates the chat model, we'd ether need to change that to do something like chatModel.extend or clone it first or pass the params as I was doing since I don't think you want a side effect of all uses of the passed chatModel to have a function_call here.

@rileytomasek
Copy link
Contributor Author

Good catch. I will change it to use clone until you get around to transitioning from being mutable to using extend immutably.

@rileytomasek rileytomasek merged commit c56a414 into master Dec 3, 2023
9 checks passed
@rileytomasek rileytomasek deleted the cleanup-duplicate-args branch December 3, 2023 00:21
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.

2 participants