improve css & svg caching #1278
Conversation
<!-- ELLIPSIS_HIDDEN --> > [!IMPORTANT] > Improves CSS and SVG caching with retry logic, error handling, and flexible expiration in `redis.py`, `agent_functions.py`, and `base.py`. > > - **Caching Improvements**: > - `RedisCache.set()` in `redis.py` and `BaseCache.set()` in `base.py` now accept an optional expiration parameter `ex`. > - **SVG and CSS Conversion**: > - Added retry logic for `_convert_svg_to_string()` and `_convert_css_shape_to_string()` in `agent_functions.py`. > - Introduced `SVG_SHAPE_CONVERTION_ATTEMPTS` and `CSS_SHAPE_CONVERTION_ATTEMPTS` constants for retry limits. > - Caches invalid shapes with a short expiration to prevent immediate retries. > - **Error Handling**: > - Handles `LLMProviderError` specifically in conversion functions in `agent_functions.py`. > - Logs detailed error information and retries after a delay. > > <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=Skyvern-AI%2Fskyvern-cloud&utm_source=github&utm_medium=referral)<sup> for 705326aee2c4da6c2804ba0dff6d1008eba91cbe. It will automatically update as commits are pushed.</sup> <!-- 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. Incremental review on 17ba333 in 48 seconds
More details
- Looked at
170lines of code in3files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_YyEBfmEzgjZSol7I
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.
| @@ -16,5 +17,5 @@ async def get(self, key: str) -> Any: | |||
| await self.set(key, value) | |||
| return value | |||
|
|
|||
| async def set(self, key: str, value: Any) -> None: | |||
| async def set(self, key: str, value: Any, ex: Union[int, timedelta, None] = CACHE_EXPIRE_TIME) -> None: | |||
There was a problem hiding this comment.
The set method does not utilize the ex parameter for setting expiration times. Consider using ex to set the TTL for each cache entry, ensuring consistent expiration behavior.
skyvern/forge/agent_functions.py
Outdated
| @@ -167,6 +184,9 @@ async def _convert_svg_to_string( | |||
| element_id=element_id, | |||
| retry=retry, | |||
| ) | |||
| if retry == CSS_SHAPE_CONVERTION_ATTEMPTS - 1: | |||
There was a problem hiding this comment.
The retry logic for Exception in _convert_svg_to_string uses CSS_SHAPE_CONVERTION_ATTEMPTS instead of SVG_SHAPE_CONVERTION_ATTEMPTS. This seems incorrect and should be corrected for consistency.
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 17ba333 in 2 minutes and 3 seconds
More details
- Looked at
170lines of code in3files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. skyvern/forge/agent_functions.py:187
- Draft comment:
The retry logic for caching invalid shapes is inconsistent. In_convert_svg_to_string, the cache expiration is set to 1 week for general exceptions, while in_convert_css_shape_to_string, it's set to 1 hour. Consider making these consistent. - Reason this comment was not posted:
Comment was on unchanged code.
2. skyvern/forge/agent_functions.py:302
- Draft comment:
The retry logic for caching invalid shapes is inconsistent. In_convert_svg_to_string, the cache expiration is set to 1 week for general exceptions, while in_convert_css_shape_to_string, it's set to 1 hour. Consider making these consistent. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_1xiRgP52wQJiDljf
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.
| @@ -16,5 +17,5 @@ async def get(self, key: str) -> Any: | |||
| await self.set(key, value) | |||
| return value | |||
|
|
|||
| async def set(self, key: str, value: Any) -> None: | |||
| async def set(self, key: str, value: Any, ex: Union[int, timedelta, None] = CACHE_EXPIRE_TIME) -> None: | |||
| self.cache[key] = value | |||
There was a problem hiding this comment.
The set method does not utilize the ex parameter for setting expiration times. Consider using ex to set the TTL for each cache entry.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on b9f558f in 16 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. skyvern/forge/agent_functions.py:189
- Draft comment:
The expiration time for caching invalid shapes is inconsistent between SVG and CSS conversion functions. Consider usingtimedelta(hours=1)for both to maintain consistency. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_TZQLoOHCG8BaAeng
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Important
Improves CSS and SVG caching with retry logic, error handling, and flexible expiration in
redis.py,agent_functions.py, andbase.py.RedisCache.set()andBaseCache.set()now accept an optional expiration parameterex._convert_svg_to_string()and_convert_css_shape_to_string()inagent_functions.py.SVG_SHAPE_CONVERTION_ATTEMPTSandCSS_SHAPE_CONVERTION_ATTEMPTSconstants for retry limits.LLMProviderErrorspecifically in conversion functions inagent_functions.py.This description was created by
for b9f558f. It will automatically update as commits are pushed.