-
Notifications
You must be signed in to change notification settings - Fork 10
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
215 - feat: discord integration - part 1 #217
Conversation
f9302a1
to
a52ebbc
Compare
* Upgrade PromptLayer and switch to .run() method * Simplify code * Cleaning * Removing old OpenAI client interface * Update message format * Cleaning * Cleaning
thenewboston/discord/utils/bot.py
Outdated
return # Ignore messages from the bot itself | ||
|
||
User = get_user_model() | ||
user = await sync_to_async(User.objects.get)(id=1) |
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.
Need to fix this (id=1)
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.
Added comments. Maybe I should work on some of them myself. Two major issue I see:
- Keep OpenAIClient since it is a good pattern to use (it encapsulates integration logic)
- Add unittests for the changes made
OpenAI = promptlayer.openai.OpenAI | ||
promptlayer_client = PromptLayer(api_key=settings.PROMPTLAYER_API_KEY) | ||
OpenAI = promptlayer_client.openai.OpenAI | ||
client = OpenAI() |
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 client = OpenAI()
used anywhere?
from thenewboston.general.models import CreatedModified | ||
from thenewboston.general.utils.transfers import change_wallet_balance | ||
|
||
promptlayer_client = PromptLayer(api_key=settings.PROMPTLAYER_API_KEY) |
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.
Let's not create objects on module import. Promptplayer did a good thing deglobalizing the configuration of the client. Now we can move PromptLayer(api_key=settings.PROMPTLAYER_API_KEY)
creation into OpenAIClient
I can work on refactoring it if you want
@@ -13,8 +12,9 @@ | |||
|
|||
from ..serializers.openai_image import OpenAIImageSerializer | |||
|
|||
promptlayer.api_key = settings.PROMPTLAYER_API_KEY | |||
OpenAI = promptlayer.openai.OpenAI | |||
promptlayer_client = PromptLayer(api_key=settings.PROMPTLAYER_API_KEY) |
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.
Let's not create objects on module import
@@ -69,15 +72,21 @@ def assess(self, save=True): | |||
assessment_points = pull.assessment_points | |||
assessment_explanation = pull.assessment_explanation | |||
case ContributionType.MANUAL.value: | |||
result = OpenAIClient.get_instance().get_chat_completion( | |||
settings.GITHUB_MANUAL_CONTRIBUTION_ASSESSMENT_TEMPLATE_NAME, | |||
response = promptlayer_client.run( |
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.
Let's keep the logic inside OpenAIClient
. So there is just one point of integration to deal with
thenewboston/discord/admin.py
Outdated
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.
let's not have this empty file
thenewboston/discord/tests.py
Outdated
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.
let's not have this empty file
thenewboston/discord/utils/bot.py
Outdated
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.
would rather have it in thenewboston/discord/client.py
or thenewboston/general/clients/discord.py
thenewboston/discord/views.py
Outdated
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.
let's not have this empty file
@@ -1,50 +0,0 @@ | |||
import json |
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.
Let's not delete this. It is very useful for testing and debugging.
return response.choices[0].message.content | ||
|
||
|
||
class OpenAIClient: |
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.
Let's keep dedicated wrapper class client approach. This is very convenient for integration purposes, since you always know the integration point and we to place common conversion logic (between more general interface of official client and what we actually need for our app). It encapsulates integration logic
Also unitests are failing |
cbd0012
to
7bab5dc
Compare
7bab5dc
to
b6b9cbb
Compare
13e18b3
to
86bd1b4
Compare
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Closes #215
Notes
Response Format
value of the prompt, therefore set it tojson
for the prompts that require machine readable results. At the moment they aregithub-pr-assessment
andmanual-assessment
discord_user_id
anddiscord_username
is implemented only on database level and Django admin. Leaving API support for this field as follow up work