Skip to content

Messenger Interface rework#357

Open
ZergLev wants to merge 55 commits intodeeppavlov:devfrom
ZergLev:feat/graceful_termination
Open

Messenger Interface rework#357
ZergLev wants to merge 55 commits intodeeppavlov:devfrom
ZergLev:feat/graceful_termination

Conversation

@ZergLev
Copy link
Collaborator

@ZergLev ZergLev commented Jun 8, 2024

Description

  • Changed the structure of PollingMessengerInterface class. Now requests are put in a queue, then done by 'workers', so that requests from several context_id's can be processed asynchronously. And a few other reasons that are explained in the Triggers PR, I think.
  • Added graceful termination in MessengerInterface. When SIGINT is received, instead of abruptly terminating, shutdown() from MessengerInterface is called, gracefully shutting down the program. All requests received prior to that are processed normally, then the program shuts down.
  • Made an async wrapper in MessengerInterface base class, so that connect() is run as an async task, not blocking the code.

Checklist

  • Move ContextLock() definition to Pipeline, probably.
  • Make graceful termination work, for some reason it kinda doesn't right now
  • Finish unit-tests (test_graceful_termination() + test_worker_shielding())
  • Get the workers to run in threads (Optional) (Right now it's commented out, because it doesn't work)
  • I have performed a self-review of the changes

To Consider

  • PollingMessengerInterface changed required "request/respond" methods, they must be changed in the child classes as well.
  • connect() methods for different Interface classes require different arguments, which is handled by a bunch of "if" statements in run_in_foreground() at the MessengerInterface class. There has to be a better way.
  • Add more tests. This is a core part of the program unit-tested only by a few simple tests. Although, I don't know, maybe this is good as is.
  • Update API reference / tutorials / guides
  • Search for references to changed entities in the codebase

@RLKRo RLKRo self-requested a review June 26, 2024 19:03
@RLKRo RLKRo mentioned this pull request Jun 27, 2024
2 tasks
ZergLev and others added 3 commits July 1, 2024 14:04
# Conflicts:
#	chatsky/messengers/common/interface.py
#	chatsky/pipeline/pipeline/pipeline.py
#	chatsky/pipeline/types.py
#	poetry.lock
#	tests/pipeline/test_messenger_interface.py
Copy link
Member

@RLKRo RLKRo left a comment

Choose a reason for hiding this comment

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

Could you also do a self-review of these changes?

I enabled CI on this PR, make sure it passes.

Didn't review the last two test cases -- need more doc to understand how the BaseTestPollingInterface should be used and how the changes made to its method assist in testing.

I think the task of inheriting tg interface from this one should be done here instead of #365, so I'll close it. I think it's better to test this rework on a real interface before merging.

Also, I think it is worth considering moving #332 in this PR as well. Multiple messenger interfaces feature would be very convenient for testing this implementation.

Comment on lines +40 to +43
def not_obtained_updates(self):
if len(self.received_updates) == len(self.expected_updates):
self.obtained_updates = True
return not self.obtained_updates
Copy link
Member

Choose a reason for hiding this comment

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

Why not assert self.expected_updates == self.recieved_updates?

Add docs to make purpose of the method clear.

Comment on lines +35 to +36
self.requests = []
self.expected_updates = []
Copy link
Member

Choose a reason for hiding this comment

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

These should be passed in __init__.

Comment on lines +147 to +148
while self.running:
await asyncio.sleep(0.05)
Copy link
Member

Choose a reason for hiding this comment

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

Add comments on the purpose of these changes to the methods (what they do for the test).

@RLKRo RLKRo linked an issue Sep 16, 2024 that may be closed by this pull request
@RLKRo RLKRo added this to the Release v1.1 milestone Nov 7, 2024
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.

[BUG] Unnecessarily verbose asyncio-related messages after termination

2 participants