Bugfix: Added changes to update prompt for webapps #306
Conversation
venkatajagannath
requested review from
Lee-W,
pankajastro,
sunank200 and
davidgxue
as code owners
Deploying with
|
| Latest commit: |
8c2899d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://36472956.ask-astro.pages.dev |
| Branch Preview URL: | https://user-venkat-bugfix.ask-astro.pages.dev |
|
Thank you Venkat for the quick turn around. Just a quick question: were you able to test that the new prompt works by producing a new URL format in the response generated? |
Hey David, please see screenshot |
There was a problem hiding this comment.
Found 1 issue that is causing an error in the main branch.
Also, can we verify this change works with firestore? Maybe historical data will not be backfilled which is fine, but are we able to know that for new requests new docs in the firestore db has this new field?
| @@ -105,4 +107,5 @@ def from_dict(cls, response_dict: dict[str, Any]) -> AskAstroRequest: | |||
| response_received_at=response_dict.get("response_received_at"), | |||
| is_processed=response_dict.get("is_processed", False), | |||
| is_example=response_dict.get("is_example", False), | |||
| client=response_dict["client"], | |||
There was a problem hiding this comment.
This should be something like response_dict.get("client", "unknown") since there's no backfill for current firestore data, meaning previous firestore data will not have this field. Currently main branch due to this PR merged is getting this error ERROR:ask_astro.rest.controllers.list_recent_requests:Error while fetching recent requests: 'client' because of it.
I can make a new PR to fix this
…312) ### Description - A previous PR didn't test thoroughly and had a bug so that it did not address the problem. - Previous PR: #306 - Goal is to have different system prompt for different slack vs webapp client - The previous PR was modifying the user's question prompt not system prompt ### Related Issue closes #301 This issue is supposed to be fixed in 0.3.0, but due to inadequate testing during that PR merge, it wasn't caught that the PR didn't address the problem. The bug is re-discovered after 0.3.0 release. ### Tests 1. Sent a request on front end 2. Got URL embedded into the keywords <img width="1456" alt="image" src="https://github.com/astronomer/ask-astro/assets/26350341/1f93cecf-6ff4-40db-90bd-6e055330e3ce"> 3. Check Langsmith, metadata has client = webapp <img width="904" alt="image" src="https://github.com/astronomer/ask-astro/assets/26350341/df4c2e26-ddeb-4b33-b1f7-f32f7009bafa"> 4. Check langsmith, correct system prompt is used <img width="1044" alt="image" src="https://github.com/astronomer/ask-astro/assets/26350341/3245201d-73ed-4ebc-b4fe-7deff770cc4a"> [Langsmith link](https://smith.langchain.com/public/37f44ef4-af11-45a5-8246-e7f2b1997b58/r)
The PR makes the following changes: