-
Notifications
You must be signed in to change notification settings - Fork 161
feat(BA-1428): Refactor event dispatcher and handlers directory structure #4497
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the event dispatcher and handler structure, extracting scheduling logic into a dedicated handler, updating import paths to match the new directory layout, and adjusting the scheduler dispatcher initialization.
- Introduce
ScheduleEventHandler
and move scheduling event subscriptions out ofSchedulerDispatcher
- Update
SchedulerDispatcher
to remove direct event consumption and adjust method signatures - Update relative import paths in
event_dispatcher/handlers/*
and adjust context fixtures for tests
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
tests/manager/conftest.py | Add event_dispatcher_test_ctx fixture and import EventDispatcher |
tests/manager/api/*.py | Replace usages of the old event_dispatcher_ctx fixture with the new one |
src/ai/backend/manager/server.py | Pass scheduler_dispatcher into dispatcher args; refactor sched_dispatcher_ctx |
src/ai/backend/manager/scheduler/dispatcher.py | Remove event consumption; change handler signatures to accept event_name |
src/ai/backend/manager/event_dispatcher/handlers/*.py | Update relative imports from two- to three-dot paths after directory move |
src/ai/backend/manager/event_dispatcher/dispatch.py | Wire up ScheduleEventHandler in Dispatchers ; consolidate bgtask logic |
src/ai/backend/manager/event_dispatcher/handlers/schedule.py | New file: implement handlers that delegate to SchedulerDispatcher |
src/ai/backend/manager/config/unified.py | Update example values for IdleCheckerConfig |
src/ai/backend/manager/api/context.py | Add scheduler_dispatcher attribute to RootContext |
Comments suppressed due to low confidence (1)
src/ai/backend/manager/event_dispatcher/handlers/schedule.py:61
- The
update_session_status
method now requires anevent_name
argument; passev.event_name()
to this call (e.g.,await self._scheduler_dispatcher.update_session_status(ev.event_name())
).
await self._scheduler_dispatcher.update_session_status()
SessionEnqueuedEvent, | ||
SessionTerminatedEvent, | ||
) | ||
from ai.backend.logging.utils import BraceStyleAdapter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Import path for BraceStyleAdapter
is inconsistent with other handlers; consider importing from ai.backend.logging
to match existing usage.
from ai.backend.logging.utils import BraceStyleAdapter | |
from ai.backend.logging import BraceStyleAdapter |
Copilot uses AI. Check for mistakes.
@@ -277,6 +278,25 @@ | |||
return argument_binding_ctx | |||
|
|||
|
|||
@pytest.fixture | |||
def event_dispatcher_test_ctx(): | |||
# TODO: Remove this fixture when the root context is refactored |
Check notice
Code scanning / devskim
A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note test
resolves #4490 (BA-1428)
Checklist: (if applicable)
ai.backend.test
docs
directory