Skip to content

Conversation

@kwonkwonn
Copy link
Contributor

resolves #8510 (BA-4189)

Move unnecessary functions which should not be located in api layer(not called from api but only from service) into service layer.

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

Copilot AI review requested due to automatic review settings February 2, 2026 05:41
@github-actions github-actions bot added size:L 100~500 LoC comp:manager Related to Manager component labels Feb 2, 2026
@kwonkwonn kwonkwonn force-pushed the refactor/fix-cyclic-import-session branch from 0d22aec to 4eb146c Compare February 2, 2026 05:47
Copy link
Contributor

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

Refactors session-related helper utilities out of the API layer into the session service layer to prevent cyclic imports (api ↔ service) and better align module responsibilities.

Changes:

  • Added services/session/utils.py and moved drop_undefined() there.
  • Moved overwritten_param_check out of api/session.py into services/session/types.py.
  • Updated services/session/service.py imports accordingly and removed the old helper definitions from api/session.py.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

File Description
src/ai/backend/manager/services/session/utils.py Introduces a service-layer utility module containing drop_undefined().
src/ai/backend/manager/services/session/types.py Hosts overwritten_param_check alongside existing session service dataclasses.
src/ai/backend/manager/services/session/service.py Switches imports from API-layer helpers to service-layer helpers.
src/ai/backend/manager/api/session.py Removes helper definitions/imports that were contributing to API↔service coupling.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +11 to +15
from ai.backend.common import validators as tx
from ai.backend.common.types import ClusterMode, SessionTypes
from ai.backend.manager.data.session.types import SessionStatus

overwritten_param_check = t.Dict({
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

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

services/session/types.py now pulls in trafaret/validators just to host overwritten_param_check. In this codebase, service types.py files appear to be kept as lightweight dataclass/type holders (e.g. services/vfolder/types.py, services/image/types.py), and services/session/types.py is imported by action modules that only need the dataclasses. Consider moving overwritten_param_check into a dedicated module (e.g. services/session/validators.py or schemas.py) so non-validation callers don’t pay the extra import-time dependency/overhead.

Copilot uses AI. Check for mistakes.
Comment on lines +12 to +14
if isinstance(v, (Mapping, dict)):
newval = drop_undefined(dict(v))
if len(newval.keys()) > 0: # exclude empty dict always
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

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

In drop_undefined(), isinstance(v, (Mapping, dict)) is redundant because dict is already a Mapping, and len(newval.keys()) > 0 does extra work. You can simplify to check only Mapping and use a truthiness check (if newval) to avoid allocating the keys() view and computing its length.

Suggested change
if isinstance(v, (Mapping, dict)):
newval = drop_undefined(dict(v))
if len(newval.keys()) > 0: # exclude empty dict always
if isinstance(v, Mapping):
newval = drop_undefined(dict(v))
if newval: # exclude empty dict always

Copilot uses AI. Check for mistakes.
@HyeockJinKim HyeockJinKim added this pull request to the merge queue Feb 3, 2026
Merged via the queue into main with commit 88027b0 Feb 3, 2026
33 checks passed
@HyeockJinKim HyeockJinKim deleted the refactor/fix-cyclic-import-session branch February 3, 2026 04:18
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.

Move util function in session.py in proper place

4 participants