-
Notifications
You must be signed in to change notification settings - Fork 22
fix: updates count tool #254
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
query: z | ||
.record(z.string(), z.unknown()) | ||
.optional() | ||
.describe("Alternative old name for filter. Will be used in db.collection.countDocuments()"), |
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.
is there a reason to keep providing this field?
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.
yes, since the LLM was already confusing query/filter, I figure that it can get confused the other way around and try to send it as "query" because it thinks we'll use .count() (even though we call out is countDocments, but who knows, it might have outdated data about MDB)
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 doesn't make much sense to me. I think we should tweak the description of filter
rather than accept two arguments that are doing the same thing.
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.
np, I'll remove and update the description
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.
we can try that for now and see if it improves, based on the issue I'm not sure the LLM will stop getting confused
countDocuments()
query
orfilter
in the tool, since LLMs might get confused about the right argument to use