-
Notifications
You must be signed in to change notification settings - Fork 71
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
Add Retry Logic to http requests #151
Changes from 9 commits
91cfb63
0399118
d11677d
645f602
5b272da
318cdc7
f7e0cb1
84ff08c
c044b58
42f503c
dc76011
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ __pycache__/ | |
*.pyc | ||
*.pyo | ||
.python-version | ||
api/api.egg-info/* | ||
|
||
# Visual Studio Code | ||
.vscode/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,82 +1,119 @@ | ||
from fastapi import APIRouter | ||
from fastapi.responses import StreamingResponse | ||
from fastapi import APIRouter, HTTPException, BackgroundTasks | ||
from fastapi.responses import StreamingResponse, JSONResponse | ||
|
||
from api import schemas | ||
from api.dependencies import app, honcho | ||
|
||
from agent.chain import ThinkCall, RespondCall | ||
|
||
import logging | ||
|
||
router = APIRouter(prefix="/api", tags=["chat"]) | ||
|
||
|
||
@router.post("/stream") | ||
async def stream( | ||
inp: schemas.ConversationInput, | ||
): | ||
"""Stream the response too the user, currently only used by the Web UI and has integration to be able to use Honcho is not anonymous""" | ||
user = honcho.apps.users.get_or_create(app_id=app.id, name=inp.user_id) | ||
async def stream(inp: schemas.ConversationInput, background_tasks: BackgroundTasks): | ||
"""Stream the response to the user, currently only used by the Web UI and has integration to be able to use Honcho if not anonymous""" | ||
try: | ||
user = honcho.apps.users.get_or_create(app_id=app.id, name=inp.user_id) | ||
|
||
def convo_turn(): | ||
thought_stream = ThinkCall( | ||
user_input=inp.message, | ||
app_id=app.id, | ||
user_id=user.id, | ||
session_id=str(inp.conversation_id), | ||
honcho=honcho, | ||
).stream() | ||
thought = "" | ||
for chunk in thought_stream: | ||
thought += chunk | ||
yield chunk | ||
def convo_turn(): | ||
thought = "" | ||
response = "" | ||
try: | ||
thought_stream = ThinkCall( | ||
user_input=inp.message, | ||
app_id=app.id, | ||
user_id=user.id, | ||
session_id=str(inp.conversation_id), | ||
honcho=honcho, | ||
).stream() | ||
for chunk in thought_stream: | ||
thought += chunk | ||
yield chunk | ||
|
||
yield "❀" | ||
response_stream = RespondCall( | ||
user_input=inp.message, | ||
thought=thought, | ||
app_id=app.id, | ||
user_id=user.id, | ||
session_id=str(inp.conversation_id), | ||
honcho=honcho, | ||
).stream() | ||
for chunk in response_stream: | ||
response += chunk | ||
yield chunk | ||
yield "❀" | ||
except Exception as e: | ||
logging.error(f"Error during streaming: {str(e)}") | ||
yield f"Error: {str(e)}" | ||
return | ||
|
||
background_tasks.add_task( | ||
create_messages_and_metamessages, | ||
app.id, user.id, inp.conversation_id, inp.message, thought, response | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See comment about concerns with this approach There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, something like this perhaps? Handling it async in the try/except should add logging for us and properly utilize the retry logic from the client with the http errors. |
||
|
||
return StreamingResponse(convo_turn()) | ||
except Exception as e: | ||
logging.error(f"An error occurred: {str(e)}") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might just be I don't know how these work, but is there a change that one of these errors could occur after the generator is finished and the messages and metamessages are created? Unclear on the relationship between the try catch block and the Streaming Response method . If there is an error in the middle of the stream what happens? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very good questions! My latest commit employs a background process for the honcho calls to separately handle/log any potential errors while (ideally) not interfering with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My worry with this approach is that if there is an error while saving a message to honcho then that is not propagated to the front-end / user. Meaning it will look like their message sent and the conversation is fine, but if they reload the messages will be gone without any indication of an error occurring. It might make sense to make separate try catch blocks or separate Exceptions for the different types of errors that the LLM vs the honcho calls are making and report them to the user differently, without using the background tasks |
||
if "rate limit" in str(e).lower(): | ||
return JSONResponse( | ||
status_code=429, | ||
content={"error": "rate_limit_exceeded", "message": "Rate limit exceeded. Please try again later."} | ||
) | ||
else: | ||
return JSONResponse( | ||
status_code=500, | ||
content={"error": "internal_server_error", "message": "An internal server error has occurred."} | ||
) | ||
|
||
yield "❀" | ||
response_stream = RespondCall( | ||
user_input=inp.message, | ||
thought=thought, | ||
|
||
@router.get("/thought/{message_id}") | ||
async def get_thought(conversation_id: str, message_id: str, user_id: str): | ||
try: | ||
user = honcho.apps.users.get_or_create(app_id=app.id, name=user_id) | ||
thought = honcho.apps.users.sessions.metamessages.list( | ||
session_id=conversation_id, | ||
app_id=app.id, | ||
user_id=user.id, | ||
session_id=str(inp.conversation_id), | ||
honcho=honcho, | ||
).stream() | ||
response = "" | ||
for chunk in response_stream: | ||
response += chunk | ||
yield chunk | ||
yield "❀" | ||
message_id=message_id, | ||
metamessage_type="thought", | ||
) | ||
# In practice, there should only be one thought per message | ||
return {"thought": thought.items[0].content if thought.items else None} | ||
except Exception as e: | ||
logging.error(f"An error occurred: {str(e)}") | ||
return JSONResponse( | ||
status_code=500, | ||
content={"error": "internal_server_error", "message": "An internal server error has occurred."} | ||
) | ||
|
||
|
||
def create_messages_and_metamessages(app_id, user_id, conversation_id, user_message, thought, ai_response): | ||
try: | ||
honcho.apps.users.sessions.messages.create( | ||
is_user=True, | ||
session_id=str(inp.conversation_id), | ||
app_id=app.id, | ||
user_id=user.id, | ||
content=inp.message, | ||
session_id=str(conversation_id), | ||
app_id=app_id, | ||
user_id=user_id, | ||
content=user_message, | ||
) | ||
new_ai_message = honcho.apps.users.sessions.messages.create( | ||
is_user=False, | ||
session_id=str(inp.conversation_id), | ||
app_id=app.id, | ||
user_id=user.id, | ||
content=response, | ||
session_id=str(conversation_id), | ||
app_id=app_id, | ||
user_id=user_id, | ||
content=ai_response, | ||
) | ||
honcho.apps.users.sessions.metamessages.create( | ||
app_id=app.id, | ||
session_id=str(inp.conversation_id), | ||
user_id=user.id, | ||
app_id=app_id, | ||
session_id=str(conversation_id), | ||
user_id=user_id, | ||
message_id=new_ai_message.id, | ||
metamessage_type="thought", | ||
content=thought, | ||
) | ||
return StreamingResponse(convo_turn()) | ||
|
||
@router.get("/thought/{message_id}") | ||
async def get_thought(conversation_id: str, message_id: str, user_id: str): | ||
user = honcho.apps.users.get_or_create(app_id=app.id, name=user_id) | ||
thought = honcho.apps.users.sessions.metamessages.list( | ||
session_id=conversation_id, | ||
app_id=app.id, | ||
user_id=user.id, | ||
message_id=message_id, | ||
metamessage_type="thought" | ||
) | ||
# In practice, there should only be one thought per message | ||
return {"thought": thought.items[0].content if thought.items else None} | ||
except Exception as e: | ||
logging.error(f"Error in background task: {str(e)}") |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,9 +6,6 @@ import dynamic from "next/dynamic"; | |
|
||
import banner from "@/public/bloom2x1.svg"; | ||
import darkBanner from "@/public/bloom2x1dark.svg"; | ||
import MessageBox from "@/components/messagebox"; | ||
import Sidebar from "@/components/sidebar"; | ||
import MarkdownWrapper from "@/components/markdownWrapper"; | ||
import { DarkModeSwitch } from "react-toggle-dark-mode"; | ||
import { FaLightbulb, FaPaperPlane, FaBars } from "react-icons/fa"; | ||
import Swal from "sweetalert2"; | ||
|
@@ -22,7 +19,15 @@ import { getSubscription } from "@/utils/supabase/queries"; | |
import { API } from "@/utils/api"; | ||
import { createClient } from "@/utils/supabase/client"; | ||
|
||
const Thoughts = dynamic(() => import("@/components/thoughts")); | ||
const Thoughts = dynamic(() => import("@/components/thoughts"), { | ||
ssr: false, | ||
}); | ||
const MessageBox = dynamic(() => import("@/components/messagebox"), { | ||
ssr: false, | ||
}); | ||
const Sidebar = dynamic(() => import("@/components/sidebar"), { | ||
ssr: false, | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick but wouldn't these components both be used immediately? Not sure what performance gain comes from this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, on further inspection, dynamically importing |
||
|
||
const URL = process.env.NEXT_PUBLIC_API_URL; | ||
|
||
|
@@ -79,11 +84,9 @@ export default function Home() { | |
const sub = await getSubscription(supabase); | ||
setIsSubscribed(!!sub); | ||
} | ||
|
||
})(); | ||
}, [supabase, posthog, userId]); | ||
|
||
|
||
useEffect(() => { | ||
const messageContainer = messageContainerRef.current; | ||
if (!messageContainer) return; | ||
|
@@ -204,7 +207,6 @@ export default function Home() { | |
isThinking = false; | ||
continue; | ||
} | ||
console.log(value) | ||
setThought((prev) => prev + value); | ||
} else { | ||
if (value.includes("❀")) { | ||
|
@@ -300,8 +302,7 @@ export default function Home() { | |
isUser={message.isUser} | ||
userId={userId} | ||
URL={URL} | ||
messageId={message.id} | ||
text={message.text} | ||
message={message} | ||
loading={messagesLoading} | ||
conversationId={conversationId} | ||
setThought={setThought} | ||
|
@@ -310,7 +311,7 @@ export default function Home() { | |
)) || ( | ||
<MessageBox | ||
isUser={false} | ||
text="" | ||
message={{ id: "", text: "" }} | ||
loading={true} | ||
setThought={setThought} | ||
setIsThoughtsOpen={setIsThoughtsOpen} | ||
|
@@ -331,9 +332,14 @@ export default function Home() { | |
{/* TODO: validate input */} | ||
<textarea | ||
ref={input} | ||
placeholder={isSubscribed ? "Type a message..." : "Subscribe to send messages"} | ||
className={`flex-1 px-3 py-1 lg:px-5 lg:py-3 bg-gray-100 dark:bg-gray-800 text-gray-400 rounded-2xl border-2 resize-none ${canSend && isSubscribed ? "border-green-200" : "border-red-200 opacity-50" | ||
}`} | ||
placeholder={ | ||
isSubscribed ? "Type a message..." : "Subscribe to send messages" | ||
} | ||
className={`flex-1 px-3 py-1 lg:px-5 lg:py-3 bg-gray-100 dark:bg-gray-800 text-gray-400 rounded-2xl border-2 resize-none ${ | ||
canSend && isSubscribed | ||
? "border-green-200" | ||
: "border-red-200 opacity-50" | ||
}`} | ||
rows={1} | ||
disabled={!isSubscribed} | ||
onKeyDown={(e) => { | ||
|
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.
Only nitpick is you can probably get rid of the background tasks import here, but looks good now