Skip to content

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

Merged
merged 3 commits into from
May 26, 2025

Conversation

HyeockJinKim
Copy link
Collaborator

resolves #4490 (BA-1428)

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version
  • Mention to the original issue
  • Installer updates including:
    • Fixtures for db schema changes
    • New mandatory config options
  • Update of end-to-end CLI integration tests in ai.backend.test
  • API server-client counterparts (e.g., manager API -> client SDK)
  • Test case(s) to:
    • Demonstrate the difference of before/after
    • Demonstrate the flow of abstract/conceptual models with a concrete implementation
  • Documentation
    • Contents in the docs directory
    • docstrings in public interfaces and type annotations

@HyeockJinKim HyeockJinKim self-assigned this May 24, 2025
@Copilot Copilot AI review requested due to automatic review settings May 24, 2025 10:07
@github-actions github-actions bot added size:L 100~500 LoC comp:manager Related to Manager component labels May 24, 2025
Copy link
Contributor

@Copilot Copilot AI left a 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 of SchedulerDispatcher
  • 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 an event_name argument; pass ev.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
Copy link
Preview

Copilot AI May 24, 2025

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.

Suggested change
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

Suspicious comment
@HyeockJinKim HyeockJinKim added this pull request to the merge queue May 26, 2025
Merged via the queue into main with commit f1da4f2 May 26, 2025
29 checks passed
@HyeockJinKim HyeockJinKim deleted the feat/separate-dispatcher-logic branch May 26, 2025 06:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:manager Related to Manager component size:L 100~500 LoC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Separate event dispatcher in manager
1 participant