korbit-ai[bot] commented on code in PR #32156:
URL: https://github.com/apache/superset/pull/32156#discussion_r1944978490
##########
superset/charts/api.py:
##########
@@ -623,7 +623,7 @@ def build_response(status_code: int) -> WerkzeugResponse:
if cache_payload.should_trigger_task(force):
logger.info("Triggering screenshot ASYNC")
- screenshot_obj.cache.set(cache_key, ScreenshotCachePayload())
+ screenshot_obj.cache.set(cache_key,
ScreenshotCachePayload().to_dict())
Review Comment:
### Unnecessary Object Creation Before Serialization <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Creating a ScreenshotCachePayload instance just to immediately convert it to
a dictionary introduces unnecessary object creation and serialization overhead.
###### Why this matters
When this operation is performed frequently, the creation and immediate
destruction of temporary objects can impact memory usage and create additional
garbage collection pressure.
###### Suggested change ∙ *Feature Preview*
Create the dictionary directly instead of creating and converting a
temporary object:
```python
screenshot_obj.cache.set(cache_key, {"status": "pending", "timestamp":
datetime.now().timestamp()})
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d6ced34-3582-46fb-8dfe-35e6dcbddb26?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:9c236027-274d-4592-aeb2-62538f963b37 -->
##########
superset/utils/screenshots.py:
##########
@@ -177,9 +201,16 @@ def get_from_cache(
def get_from_cache_key(cls, cache_key: str) -> ScreenshotCachePayload |
None:
logger.info("Attempting to get from cache: %s", cache_key)
if payload := cls.cache.get(cache_key):
- # for backwards compatability, byte objects should be converted
- if not isinstance(payload, ScreenshotCachePayload):
+ # Initially, only bytes were stored. This was changed to store an
instance
+ # of ScreenshotCachePayload, but since it can't be serialized in
all
+ # backends it was further changed to a dict of attributes.
+ if isinstance(payload, bytes):
payload = ScreenshotCachePayload(payload)
+ elif isinstance(payload, ScreenshotCachePayload):
+ pass
+ elif isinstance(payload, dict):
+ payload = cast(ScreenshotCachePayload, payload)
+ payload = ScreenshotCachePayload.from_dict(payload)
Review Comment:
### Incorrect Type Cast Before Deserialization <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The unnecessary cast to ScreenshotCachePayload before calling from_dict can
cause runtime errors since the dictionary is being incorrectly typed.
###### Why this matters
The cast operation is misleading and doesn't actually convert the dictionary
to a ScreenshotCachePayload. It could lead to runtime errors if the code tries
to use the casted payload before the from_dict conversion.
###### Suggested change ∙ *Feature Preview*
Remove the unnecessary cast and directly call from_dict on the dictionary:
```python
elif isinstance(payload, dict):
payload = ScreenshotCachePayload.from_dict(payload)
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/359a8a96-8e1b-45a5-af83-e318894219ab?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:be458e02-f008-46ac-a916-fae789cddbe0 -->
##########
superset/utils/screenshots.py:
##########
@@ -63,13 +63,37 @@
ERROR = "Error"
+class ScreenshotCachePayloadType(TypedDict):
+ image: bytes | None
+ timestamp: str
+ status: str
+
+
class ScreenshotCachePayload:
- def __init__(self, image: bytes | None = None):
+ def __init__(
+ self,
+ image: bytes | None = None,
+ status: StatusValues = StatusValues.PENDING,
+ timestamp: str = "",
+ ):
self._image = image
- self._timestamp = datetime.now().isoformat()
- self.status = StatusValues.PENDING
- if image:
- self.status = StatusValues.UPDATED
+ self._timestamp = timestamp or datetime.now().isoformat()
+ self.status = StatusValues.UPDATED if image else status
Review Comment:
### Status Override on Image Presence <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The initial status logic in ScreenshotCachePayload.__init__ overrides the
provided status parameter when an image is present, which may not be the
desired behavior in all cases.
###### Why this matters
This could lead to unexpected state transitions when initializing a payload
with both an image and a specific status, as the image presence will always
force an UPDATED status.
###### Suggested change ∙ *Feature Preview*
Consider whether to respect the provided status parameter regardless of
image presence:
```python
self.status = status
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/aee37752-9de0-4330-9d43-82d51f8aaad1?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:37f4f938-6a2e-48ae-b53a-fde4597a1d60 -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
