use cached prompt generation #768
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
<!-- ELLIPSIS_HIDDEN --> | 🚀 | This description was created by [Ellipsis](https://www.ellipsis.dev) for commit 35c5b0690e2a719346c9e241e9244349818af3f6 | |--------|--------| ### Summary: This PR implements caching for task generation using user prompt hashes, updating models, routes, and schemas for efficiency. **Key points**: - Implements caching for task generation using user prompt hashes. - Adds `user_prompt_hash` and `source_task_generation_id` to `TaskGenerationModel`. - Updates `AgentDB` to handle cached task generations. - Modifies `/generate/task` route to check cache before generating tasks. - Introduces `PROMPT_CACHE_WINDOW_HOURS` for cache duration control. - Updates `skyvern/forge/sdk/schemas/task_generations.py` to include `user_prompt_hash` in `TaskGeneration` class. - Includes Alembic migration script for database schema update. ---- Generated with ❤️ by [ellipsis.dev](https://www.ellipsis.dev) <!-- ELLIPSIS_HIDDEN -->
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to ff66020 in 25 seconds
More details
- Looked at
212lines of code in5files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. skyvern/forge/sdk/db/client.py:1426
- Draft comment:
The default value forquery_window_hoursshould besettings.PROMPT_CACHE_WINDOW_HOURSinstead ofsettings.PROMPT_ACTION_HISTORY_WINDOW. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is about a change made in the diff, specifically the default value of a parameter in a new method. The comment suggests a specific change, which is actionable. However, I need to consider if the suggested change is necessary or if the current default value is appropriate.
I might be missing the context of whysettings.PROMPT_ACTION_HISTORY_WINDOWwas chosen as the default. The current default might be intentional and changing it could affect the functionality. Without additional context, it's hard to determine if the suggested change is necessary.
The comment is actionable and specific, suggesting a change to a default value. Even if the current default is intentional, the comment highlights a potential oversight that should be considered.
Keep the comment as it is specific to a change made in the diff and suggests an actionable improvement.
Workflow ID: wflow_qTbWeajD7b71DQh2
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -54,6 +55,7 @@ | |||
| base_router = APIRouter() | |||
|
|
|||
| LOG = structlog.get_logger() | |||
| PROMPT_CACHE_WINDOW_HOURS = 24 | |||
There was a problem hiding this comment.
PROMPT_CACHE_WINDOW_HOURS is already defined in config.py. Consider importing it from there instead of redefining it.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on ff66020 in 45 seconds
More details
- Looked at
211lines of code in5files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. skyvern/forge/sdk/db/client.py:1426
- Draft comment:
PROMPT_ACTION_HISTORY_WINDOWis not defined in the provided code. Consider usingPROMPT_CACHE_WINDOW_HOURSfromskyvern/config.pyinstead. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is speculative and assumes a lack of definition forPROMPT_ACTION_HISTORY_WINDOWwithout evidence. The import ofsettingssuggests thatPROMPT_ACTION_HISTORY_WINDOWcould be defined there. The comment does not provide actionable feedback based on the diff alone.
I might be overlooking the possibility thatPROMPT_ACTION_HISTORY_WINDOWis indeed undefined insettings, but the comment does not provide evidence for this claim.
Even ifPROMPT_ACTION_HISTORY_WINDOWis undefined, the comment should provide evidence or context to support its claim. Without this, it remains speculative.
The comment is speculative and not based on evidence from the diff. It should be removed as it does not provide actionable feedback.
Workflow ID: wflow_Imr5sIGXXrYiMbrx
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| # check if there's a same user_prompt within the past x Hours | ||
| # in the future, we can use vector db to fetch similar prompts | ||
| existing_task_generation = await app.DATABASE.get_task_generation_by_prompt_hash( | ||
| user_prompt_hash=user_prompt_hash, query_window_hours=PROMPT_CACHE_WINDOW_HOURS |
There was a problem hiding this comment.
Consider using settings.PROMPT_CACHE_WINDOW_HOURS instead of the hardcoded value 24 for consistency and maintainability.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 36863f0 in 18 seconds
More details
- Looked at
52lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. alembic/versions/2024_09_03_0356-0de9150bc624_update_task_generation_table_use_user_.py:24
- Draft comment:
Consider adding a step to handle existing data for the new columnsuser_prompt_hashandsource_task_generation_id. This could involve setting default values or updating existing rows to prevent issues if these columns are expected to be non-nullable in the application logic. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is about a potential issue with the new columns being nullable. If the application logic requires non-null values, this could be a valid concern. However, the migration itself doesn't enforce non-null constraints, so the comment might be speculative.
The comment might be speculative since it assumes application logic requirements that aren't enforced by the migration. The migration allows null values, so existing data won't cause schema issues.
Even though the migration allows null values, the comment could be useful if the application logic requires non-null values, which isn't clear from the migration alone.
The comment is speculative about application logic requirements, but it could be useful if non-null values are required. However, based on the rules, speculative comments should be removed.
Workflow ID: wflow_CXWonUP8ApVDo1bS
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
This PR implements caching for task generation using user prompt hashes, updating models, routes, schemas, and includes a specific Alembic migration script for schema changes.
Key points:
user_prompt_hashandsource_task_generation_idtoTaskGenerationModelinskyvern/forge/sdk/db/models.py.AgentDBinskyvern/forge/sdk/db/client.pyto handle cached task generations./generate/taskroute inskyvern/forge/sdk/routes/agent_protocol.pyto check cache before generating tasks.PROMPT_CACHE_WINDOW_HOURSinskyvern/config.pyfor cache duration control.skyvern/forge/sdk/schemas/task_generations.pyto includeuser_prompt_hashinTaskGenerationclass.alembic/versions/2024_09_03_0356-0de9150bc624_update_task_generation_table_use_user_.pyfor database schema update.Generated with ❤️ by ellipsis.dev