-
Notifications
You must be signed in to change notification settings - Fork 31
Semantic group by 253 #255
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
Conversation
mdr223
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.
Overall -- fantastic job!! You went above and beyond what I expected; including getting started with (1) using our Generator for LLM calls w/built-in cost / latency tracking and (2) marshaling those stats into a DataRecordSet to be returned by the __call__ method.
That alone knocked out like 50-70% of what I had planned for your next steps. I'll follow-up with those details in the same issue from before.
…d non-semantic groupbys
mdr223
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.
Looks great! I just left some minor comments which should only take ~15-30 mins to resolve. Once those changes are applied I will merge dev into this branch and we can begin to prepare for merging this into main!
mdr223
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.
LGTM!
No description provided.