-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
feat(analytics): rely on sql rather that python loop for brains #2437
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
❌ Changes requested.
- Reviewed the entire pull request up to f0ad664
- Looked at
78
lines of code in1
files - Took 1 minute and 42 seconds to review
More info
- Skipped
0
files when reviewing. - Skipped posting
1
additional comments because they didn't meet confidence threshold of50%
.
1. /backend/modules/analytics/repository/analytics.py:37
:
- Assessed confidence :
80%
- Grade:
0%
- Comment:
Consider storing themessage_time
as a datetime object in the database to avoid costly string to datetime conversions. - Reasoning:
The PR author has made a good improvement by reducing the number of database queries. However, there is a potential performance issue with the way the chat history is being processed. For each chat in the chat history, the message time is being parsed from a string into a datetime object. This could be a costly operation if the chat history is large. It would be more efficient to store the message time as a datetime object in the database, so that this conversion is not necessary.
Workflow ID: wflow_XtMs5mcZRh1KHkbS
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. We'll respond in a few minutes. Learn more here.
⌛ You have 2 days remaining in your free trial. Upgrade at https://app.ellipsis.dev for $20/developer/month. If you have any questions, reach us at help@ellipsis.dev
for date in all_dates: | ||
usage_per_day[date] += 0 | ||
usage_per_day[date] += 0 |
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.
The operation usage_per_day[date] += 0
is unnecessary. The defaultdict will automatically return 0 for any key that does not exist in the dictionary.
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.
@ellipsis-dev fix this
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.
@StanGirard, I have addressed your comments in pull request #2438
…on sql rather that python loop for brains);
🤖 I have created a release *beep* *boop* --- ## 0.0.231 (2024-04-19) ## What's Changed * feat(assistants): Add user usage update and pricing calculation to ITO assistant by @StanGirard in #2433 * feat(assistant): improve prompt summary by @StanGirard in #2435 * feat(assistants): Add PDF generation functionality and nice emails by @StanGirard in #2436 * feat(analytics): rely on sql rather that python loop for brains by @StanGirard in #2437 * fix(assistant): summary now can output 2000 tokens by @StanGirard in #2440 * feat(assistant): check if key of file is same as filename uploaded by @StanGirard in #2439 * feat: Update Docker build commands and dependencies by @StanGirard in #2441 * feat(rag): Refactor DEFAULT_DOCUMENT_PROMPT in quivr_rag.py by @StanGirard in #2442 * Enable Porter Application quivr-back by @porter-deployment-app in #2443 * Enable Porter Application quivr-demo-front by @porter-deployment-app in #2444 * fix(assistants): brain id is null by @StanGirard in #2445 * feat(summary): improve prompt to get more insights by @StanGirard in #2446 * feat(aws): Update CPU and memory configurations for task definitions by @StanGirard in #2447 * feat(frontend): Quivr Assistants by @Zewed in #2448 **Full Changelog**: v0.0.230...v0.0.231 --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- ELLIPSIS_HIDDEN --> ---- | 🚀 This description was created by [Ellipsis](https://www.ellipsis.dev) for commit a16fa4d | |--------| ### Summary: This PR releases version 0.0.231, introducing several feature enhancements and bug fixes across the assistant, analytics, Docker, and frontend modules. **Key points**: - Release version 0.0.231 with feature enhancements and bug fixes across multiple modules - User usage update and pricing calculation added to ITO assistant - Improved prompt summary in assistant module - PDF generation functionality and email enhancements added - Analytics optimized by relying on SQL instead of Python loop - Token output limit fixed in assistant summary - Docker build commands and dependencies updated - DEFAULT_DOCUMENT_PROMPT in quivr_rag.py refactored - Porter Applications for quivr-back and quivr-demo-front enabled - Null brain id issue fixed in assistants module - Prompt improved for better insights in summary module - CPU and memory configurations for AWS task definitions updated - Quivr Assistants added in frontend ---- Generated with ❤️ by [ellipsis.dev](https://www.ellipsis.dev) <!-- ELLIPSIS_HIDDEN -->
This pull request refactors the get_brains_usages method in the Analytics class. The changes include improving code readability, optimizing database queries, and fixing potential bugs.
Summary:
This PR optimizes the
get_brains_usages
method in theAnalytics
class by reducing database calls and improving code readability.Key points:
get_brains_usages
method inAnalytics
class.Generated with ❤️ by ellipsis.dev