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: shutdown issues [SBK-358] #43

Closed
wants to merge 7 commits into from
Closed

Conversation

fubuloubu
Copy link
Member

What I did

There was a bunch of non-critical issues when a Silverback app is shutdown via external means

How I did it

  • upgraded taskiq
  • added some methods to unsubscribe and shutdown everything better

How to verify it

play with it

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)

@vany365 vany365 changed the title fix: shutdown issues fix: shutdown issues [SBK-358] Nov 13, 2023
Copy link

linear bot commented Nov 13, 2023

SBK-358 "fix: shutdown issues" (ApeWorX/silverback #43)

What I did

There was a bunch of non-critical issues when a Silverback app is shutdown via external means

How I did it

  • upgraded taskiq
  • added some methods to unsubscribe and shutdown everything better

How to verify it

play with it

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)

ApeWorX/silverback #43 by fubuloubu on GitHub

via LinearSync

Copy link
Member Author

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

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

@mikeshultz approve this and we can merge/release

@fubuloubu fubuloubu requested a review from mikeshultz November 13, 2023 22:20
@fubuloubu fubuloubu enabled auto-merge (squash) November 13, 2023 22:20
@mikeshultz
Copy link
Contributor

@mikeshultz approve this and we can merge/release

I'm not entirely sure if this is good to go or not. Can't run the example app. The tokenlists package requires pydantic 1. So either I'm missing something with the compat, or we need that to update for the example app to run.

@fubuloubu if you're okay with this being untested, lmk and I'll ✔️ this.

@mikeshultz
Copy link
Contributor

This seems to have some issues with pydantic v2 compatibility. TaskIQ has some internals that allow it to handle pydantic model serialization. And the way it detects which way to go is via the installed package version:

https://github.com/taskiq-python/taskiq/blob/9a6e49fa754f8645793b018a26a8a48f997ae587/taskiq/compat.py#L13-L26

When it tries to call model_dump() on the model to serialize it, it fails with this:

Traceback (most recent call last):
  File "/home/mike/.venvs/silverback/lib/python3.10/site-packages/ape/utils/basemodel.py", line 206, in __getattr__
    return super().__getattr__(name)
AttributeError: 'super' object has no attribute '__getattr__'. Did you mean: '__setattr__'?

I think this might be an issue with the __getattr__() implementation's compatibility with pydantic v2?

@mikeshultz
Copy link
Contributor

Reported in ApeWorX/ape#1736

@mikeshultz
Copy link
Contributor

Unless we do something like implement model_dump() (and maybe other v2 methods) on Ape core's BaseModel I think this might have to wait until after core has fully migrated to v2.

@fubuloubu fubuloubu disabled auto-merge November 14, 2023 07:56
@antazoey
Copy link
Member

Unless we do something like implement model_dump() (and maybe other v2 methods) on Ape core's BaseModel I think this might have to wait until after core has fully migrated to v2.

The only solution I can see it is wait for Ape 0.7 which will have proper v2 support. Sorry - I didn't realize where you were seeing this issue before but now I understand!

@fubuloubu fubuloubu closed this Apr 8, 2024
@fubuloubu fubuloubu deleted the fix/shutdown-issues branch April 8, 2024 21:12
@fubuloubu
Copy link
Member Author

closed in favor of #65

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants