Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: error checking type for "subscripted generics" #77

Merged
merged 8 commits into from
May 4, 2024

Conversation

mikeshultz
Copy link
Contributor

What I did

Fixes the error:

[...snip...]
  File "/home/mike/.venvs/silverback-cluster/lib/python3.11/site-packages/silverback/recorder.py", line 91, in from_taskiq
    metrics=cls._extract_custom_metrics(result.return_value, task_name),
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/mike/.venvs/silverback-cluster/lib/python3.11/site-packages/silverback/recorder.py", line 45, in _extract_custom_metrics
    elif isinstance(result, ScalarType):  # type: ignore[arg-type,misc]
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/typing.py", line 1689, in __instancecheck__
    return self.__subclasscheck__(type(obj))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/typing.py", line 1693, in __subclasscheck__
    if issubclass(cls, arg):
       ^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/typing.py", line 1315, in __subclasscheck__
    raise TypeError("Subscripted generics cannot be used with"
TypeError: Subscripted generics cannot be used with class and instance checks

Checklist

  • Passes all linting checks (pre-commit and CI jobs)
  • New test cases have been added and are passing
  • Documentation has been updated
  • PR title follows Conventional Commit standard (will be automatically included in the changelog)

@mikeshultz mikeshultz added the bug Something isn't working label May 4, 2024
@mikeshultz mikeshultz self-assigned this May 4, 2024
silverback/types.py Outdated Show resolved Hide resolved
fubuloubu
fubuloubu previously approved these changes May 4, 2024
Comment on lines +53 to +54
# NOTE: Interesting side effect is that `int` outside the INT96 range parse as `Decimal`
# This is okay, preferable actually, because it means we can store ints outside that range
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikeshultz this is actually kinda interesting, it will "auto-parse" values outside the INT96 range as Decimal, which can actually be (less efficiently) encoded by Parquet, avoiding raising a bug about it!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In [14]: Datapoints(root={"return_value": 1324})
Out[14]: Datapoints(root={'return_value': ScalarDatapoint(type='scalar', data=1324)})

In [15]: Datapoints(root={"return_value": 2**291})
Out[15]: Datapoints(root={'return_value': ScalarDatapoint(type='scalar', data=Decimal('3978585891278293137243057985174566720803649206378781739523711815145275976100267004264448'))})

@fubuloubu fubuloubu enabled auto-merge (squash) May 4, 2024 17:14
fubuloubu
fubuloubu previously approved these changes May 4, 2024
fubuloubu
fubuloubu previously approved these changes May 4, 2024
@fubuloubu fubuloubu disabled auto-merge May 4, 2024 17:36
@fubuloubu fubuloubu merged commit 29c24f1 into main May 4, 2024
21 checks passed
@fubuloubu fubuloubu deleted the fix/scalar-check branch May 4, 2024 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants