-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Skip deep type-validation on state var hot paths #6738
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
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 |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Skip deep element-wise type validation on state var hot paths: computed var return types are only checked on recompute (not on cache hits), and production mode no longer walks every element of assigned containers for the log-only type check. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Skip deep element-wise type validation on state var hot paths: computed var return types are only checked on recompute (not on cache hits), and production mode no longer walks every element of assigned containers for the log-only type check. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,6 +66,7 @@ | |
| GenericType, | ||
| Self, | ||
| _isinstance, | ||
| _validation_depth, | ||
| get_origin, | ||
| has_args, | ||
| safe_issubclass, | ||
|
|
@@ -2587,23 +2588,28 @@ def __get__(self, instance: BaseState | None, owner: type): | |
|
|
||
| if not self._cache: | ||
| value = self.fget(instance) | ||
| else: | ||
| # handle caching | ||
| if not hasattr(instance, self._cache_attr) or self.needs_update(instance): | ||
| # Set cache attr on state instance. | ||
| setattr(instance, self._cache_attr, self.fget(instance)) | ||
| # Ensure the computed var gets serialized to redis. | ||
| instance._was_touched = True | ||
| # Set the last updated timestamp on the state instance. | ||
| setattr(instance, self._last_updated_attr, datetime.datetime.now()) | ||
| value = getattr(instance, self._cache_attr) | ||
| self._check_deprecated_return_type(instance, value) | ||
| return value | ||
|
Collaborator
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. if we return early, how does the computed value get cached? |
||
|
|
||
| self._check_deprecated_return_type(instance, value) | ||
| # handle caching | ||
| if not hasattr(instance, self._cache_attr) or self.needs_update(instance): | ||
| # Set cache attr on state instance. | ||
| setattr(instance, self._cache_attr, self.fget(instance)) | ||
| # Ensure the computed var gets serialized to redis. | ||
| instance._was_touched = True | ||
| # Set the last updated timestamp on the state instance. | ||
| setattr(instance, self._last_updated_attr, datetime.datetime.now()) | ||
| value = getattr(instance, self._cache_attr) | ||
| # Only validate the return type when the value was just computed. | ||
| self._check_deprecated_return_type(instance, value) | ||
| return value | ||
|
|
||
| return value | ||
| return getattr(instance, self._cache_attr) | ||
|
greptile-apps[bot] marked this conversation as resolved.
Contributor
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.
|
||
|
|
||
| def _check_deprecated_return_type(self, instance: BaseState, value: Any) -> None: | ||
| if not _isinstance(value, self._var_type, nested=1, treat_var_as_type=False): | ||
| if not _isinstance( | ||
| value, self._var_type, nested=_validation_depth(), treat_var_as_type=False | ||
| ): | ||
| console.error( | ||
| f"Computed var '{type(instance).__name__}.{self._name}' must return" | ||
| f" a value of type '{escape(str(self._var_type))}', got '{value!s}' of type {type(value)}." | ||
|
|
@@ -2858,9 +2864,11 @@ async def _awaitable_result(instance: BaseState = instance) -> RETURN_TYPE: | |
| instance._was_touched = True | ||
| # Set the last updated timestamp on the state instance. | ||
| setattr(instance, self._last_updated_attr, datetime.datetime.now()) | ||
| value = getattr(instance, self._cache_attr) | ||
| self._check_deprecated_return_type(instance, value) | ||
| return value | ||
| value = getattr(instance, self._cache_attr) | ||
| # Only validate the return type when the value was just computed. | ||
| self._check_deprecated_return_type(instance, value) | ||
| return value | ||
| return getattr(instance, self._cache_attr) | ||
|
|
||
| return _awaitable_result() | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.