-
-
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(assistants): Add user usage update and pricing calculation to ITO assistant #2433
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 be9e5ab
- Looked at
50
lines of code in2
files - Took 3 minutes and 2 seconds to review
More info
- Skipped
0
files when reviewing. - Skipped posting
2
additional comments because they didn't meet confidence threshold of50%
.
1. /backend/modules/assistant/ito/ito.py:34
:
- Assessed confidence :
80%
- Grade:
0%
- Comment:
Thecalculate_pricing()
method is returning a fixed value of 20. This might not be a good practice if the pricing is subject to change or if it depends on some factors. It would be better to have a more flexible way of calculating the pricing. - Reasoning:
Thecalculate_pricing()
method is returning a fixed value of 20. This might not be a good practice if the pricing is subject to change or if it depends on some factors. It would be better to have a more flexible way of calculating the pricing.
2. /backend/modules/assistant/ito/summary.py:68
:
- Assessed confidence :
80%
- Grade:
0%
- Comment:
Theprocess_assistant()
method is catching all exceptions and returning an error message. This might not be a good practice as it can hide potential bugs and make debugging harder. It would be better to catch specific exceptions and handle them accordingly. - Reasoning:
Theprocess_assistant()
method in theSummaryAssistant
class is catching all exceptions and returning an error message. This might not be a good practice as it can hide potential bugs and make debugging harder. It would be better to catch specific exceptions and handle them accordingly.
Workflow ID: wflow_4D0ZikBsmZnNjGqt
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. We'll respond in a few minutes. Learn more here.
⌛ You have 3 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
def increase_usage_user(self): | ||
# Raises an error if the user has consumed all of of his credits | ||
|
||
update_user_usage( |
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 increase_usage_user()
method is trying to access self.user_usage
and self.user_settings
which are not defined in the class ITO
. This will raise an AttributeError when the method is called. Please define these attributes or update the method to use the correct attributes.
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.
Hi @StanGirard, I noticed that the increase_usage_user()
method in the ITO
class is trying to access self.user_usage
and self.user_settings
which are not defined in the class. This will raise an AttributeError when the method is called. Could you please clarify if user_usage
and user_settings
are attributes of current_user
or if they are independent variables that need to be passed separately to the ITO
class? Thanks!
Responding to this comment by @StanGirard. For more information about Ellipsis, check the documentation.
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.
- Performed an incremental review on ee5fdf7
- Looked at
46
lines of code in1
files - Took 3 minutes and 32 seconds to review
More info
- Skipped
0
files when reviewing. - Skipped posting
0
additional comments because they didn't meet confidence threshold of50%
.
Workflow ID: wflow_6AauWYRAeUN4m0Fx
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
email=current_user.email, | ||
) | ||
self.user_settings = self.user_usage.get_user_settings() | ||
self.increase_usage_user() |
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.
Consider moving the call to increase_usage_user
out of the constructor to avoid potential issues with instance creation if the user has consumed all their credits.
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 good to me!
- Performed an incremental review on 9ea3fc3
- Looked at
20
lines of code in1
files - Took 2 minutes and 3 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/assistant/dto/outputs.py:86
:
- Assessed confidence :
80%
- Grade:
0%
- Comment:
Consider making the cost in the Pricing class configurable instead of a fixed value. This would provide more flexibility for different pricing strategies in the future. - Reasoning:
The Pricing class has a fixed cost of 20. This might not be flexible enough for different pricing strategies in the future. It would be better to have this value configurable.
Workflow ID: wflow_JXcPJYF5k12m9Tj4
Not what you expected? You can customize the content of the reviews using rules. 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
🤖 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 adds functionality to update user usage and calculate pricing in the ITO assistant. It includes a new method
increase_usage_user()
that raises an error if the user has consumed all of their credits, and a new methodcalculate_pricing()
that returns a fixed pricing value of 20.Summary:
This PR adds functionality to update user usage and calculate pricing in the ITO assistant, with new methods in the
ITO
class, the addition of aPricing
class, and an update to where theincrease_usage_user()
method is called.Key points:
increase_usage_user()
andcalculate_pricing()
methods toITO
classincrease_usage_user()
updates user usage and raises an error if all credits are consumedcalculate_pricing()
returns a fixed pricing value of 20increase_usage_user()
is now called in theITO
class constructorPricing
class tooutputs.py
filePricing
class is used inAssistantOutput
class to provide pricing informationGenerated with ❤️ by ellipsis.dev