refactor(BA-4186): move out of convention function into valid directory by kwonkwonn · Pull Request #8509 · lablup/backend.ai

kwonkwonn · 2026-02-02T04:43:58Z

Move out of convention query_wsproxy_status function into dedicated class,
which prevent cicular import issue and clean up the code structure.
`
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

kwonkwonn · 2026-02-02T04:47:59Z

src/ai/backend/manager/clients/appproxy/client.py

@@ -36,6 +45,28 @@ def __init__(self, client_session: aiohttp.ClientSession, address: str, token: s
        self._address = address
        self._token = token

+    @staticmethod


As the purpose and how it initialze is little bit different, query_status and AppProxy,
I've put it as a method but to keep it's purpose, leave arguments and logic the same.

Copilot

Pull request overview

This PR refactors the query_wsproxy_status function by moving it from the API layer (manager/api/scaling_group.py) to the client layer (manager/clients/appproxy/client.py) as a static method AppProxyClient.query_status(). This resolves a circular import issue and improves code organization by placing the client logic in the appropriate directory.

Changes:

Moved query_wsproxy_status function to AppProxyClient.query_status() static method
Updated import statements and function calls across affected files
Improved error message terminology from "wsproxy" to "app-proxy" for consistency

Reviewed changes

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

File	Description
src/ai/backend/manager/api/scaling_group.py	Removed `query_wsproxy_status` function and related imports (`json`, `aiotools`, `InternalServerError`); added import for `AppProxyClient` and updated function call
src/ai/backend/manager/clients/appproxy/client.py	Added `query_status` static method with required imports (`json`, `logging`, `aiotools`, `BraceStyleAdapter`, `InternalServerError`)
src/ai/backend/manager/services/session/service.py	Updated import from `scaling_group` to `appproxy.client` and changed function call to use new static method

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

Copilot · 2026-02-02T04:50:54Z

src/ai/backend/manager/clients/appproxy/client.py

+    @staticmethod
+    @aiotools.lru_cache(expire_after=30)  # expire after 30 seconds
+    async def query_status(
+        appproxy_addr: str,
+    ) -> dict[str, Any]:
+        """Query the status of an app-proxy (wsproxy) at the given address."""
+        async with (
+            aiohttp.ClientSession() as session,
+            session.get(
+                appproxy_addr + "/status",
+                headers={"Accept": "application/json"},
+            ) as resp,
+        ):
+            try:
+                result = await resp.json()
+            except (aiohttp.ContentTypeError, json.JSONDecodeError) as e:
+                log.error("Failed to parse app-proxy status response from {}: {}", appproxy_addr, e)
+                raise InternalServerError(
+                    "Got invalid response from app-proxy when querying status"
+                ) from e
+            return result


The query_status method is inconsistent with other methods in this class in several ways:

Static method pattern: This is a static method that creates its own aiohttp.ClientSession, while other methods (create_endpoint, delete_endpoint) are instance methods that use self._client_session. This pattern prevents connection pooling and reuse.

Missing HTTP status check: Unlike other methods in this class (e.g., line 83: resp.raise_for_status()), this method doesn't validate the HTTP response status. Non-2xx responses will be silently processed, potentially leading to confusing errors.

Missing resilience policy: Other methods in this class use the @appproxy_client_resilience.apply() decorator (lines 70, 86) to apply retry and metrics policies, but this method doesn't have this protection.

Consider refactoring this to be an instance method that follows the established pattern in this class. However, note that this would require updating the call sites to instantiate AppProxyClient instead of calling it as a static method, which may not be feasible given the current usage pattern where an address is passed but no client instance exists.

I'm not sure modifying code logic would be in the scope for this pr.

Also, query_status usually done with passing the paramethers which were retrieved from DB_conntection.
It seems out of scope to modify that relevant functions.

moving this method into standalone function can make sense though.

Copilot · 2026-02-02T04:50:55Z

src/ai/backend/manager/clients/appproxy/client.py

+                appproxy_addr + "/status",
+                headers={"Accept": "application/json"},
+            ) as resp,
+        ):


Missing HTTP status validation. The response status should be checked before attempting to parse the JSON body. If the app-proxy returns a non-2xx status code (e.g., 404, 500), the code will attempt to parse the error response as JSON without properly handling the HTTP error.

Add resp.raise_for_status() after line 60 and before attempting to parse the response, similar to the pattern used in the create_endpoint method at line 83.

Suggested change

):

):

resp.raise_for_status()

fregataa · 2026-02-02T07:41:26Z

src/ai/backend/manager/api/scaling_group.py

@@ -109,7 +86,7 @@ async def get_wsproxy_version(request: web.Request, params: Any) -> web.Response
                    wsproxy_version = "v1"
                else:
                    try:
-                        wsproxy_status = await query_wsproxy_status(wsproxy_addr)
+                        wsproxy_status = await AppProxyClient.query_status(wsproxy_addr)


AppProxyClient should be initialized only once when starting the app and query_status() should not be static or class method.
Or you can load the client from a client pool. Please check how AppProxyClient is initialized in AgentRegistry.

Understood, AppProxyClient is class only should be called when creating or deleting Client.
But as query_status seems more like an utility function which it's role is just being used when terminating or checking the version, I guess i should just split it out of the class for now.

kwonkwonn · 2026-02-02T08:13:16Z

Minimized modification's scope as purpose of this PR is preventing circular imports

HyeockJinKim · 2026-02-03T03:55:44Z

src/ai/backend/manager/api/scaling_group.py


 from ai.backend.common import validators as tx
 from ai.backend.logging import BraceStyleAdapter
+from ai.backend.manager.clients.appproxy.client import query_appproxy_status


It wasn't about providing it as a generic function, but rather implementing it as a method in the client layer.

Can you have a look on my first commit?
I firstly tried to put it as @dataclass method, but i felt query_appproxy_status has less relevant features with other methods of AppProxyClient.

If it feels tolerable, I will work that way, or If i'm missing other better way.
please let me know!

kwonkwonn · 2026-02-03T07:50:08Z

Summary

Add AppProxyClientPool to root_ctx and ServiceArgs
Add domain-specific errors (AppProxyConnectionError, AppProxyResponseError)
Remove query_appproxy_status() (created new HTTP session on every call)
Migrate api/scaling_group.py and SessionService to use the new pool

Note

appproxy_client_pool is exposed via root_ctx for direct API access as a temporary measure
deally should be accessed only through service layer (future cleanup)
AgentRegistry still has its own _client_pool for create/delete_endpoint - gradual migration planned

fregataa · 2026-02-05T04:32:29Z

src/ai/backend/manager/clients/appproxy/client.py

+    async def fetch_status(self) -> dict[str, Any]:
+        try:
+            async with self._client_session.get(
+                "/status",
+                headers={"Accept": "application/json"},
+            ) as resp:


TODO: strict return type such as pydantic or dataclass

I was working on minor fixes on appproxy, i will refactor to this in between.

kwonkwonn · 2026-02-05T05:01:56Z

src/ai/backend/manager/api/scaling_group.py

+        client = root_ctx.appproxy_client_pool.load_client(
+            sgroup.wsproxy_addr, sgroup.wsproxy_api_token or ""
+        )
+        status = await client.fetch_status()


When this code snippet including data base connection moves to seperate module, we can remove root_ctx.appproxy_client_pool and pass it to service layer arguments only.

HyeockJinKim

OTEL related modification seems to be invalid. Could you check this.

Copilot AI review requested due to automatic review settings February 2, 2026 04:43

github-actions bot assigned kwonkwonn Feb 2, 2026

Copilot started reviewing on behalf of kwonkwonn February 2, 2026 04:44 View session

kwonkwonn commented Feb 2, 2026

View reviewed changes

Copilot AI reviewed Feb 2, 2026

View reviewed changes

kwonkwonn force-pushed the refactor/move-query-wsproxy-into-method branch from dc27a4b to def3e5a Compare February 2, 2026 05:25

kwonkwonn requested review from HyeockJinKim and fregataa February 2, 2026 06:09

fregataa requested changes Feb 2, 2026

View reviewed changes

kwonkwonn requested a review from fregataa February 2, 2026 08:12

github-actions bot added size:M 30~100 LoC comp:manager Related to Manager component labels Feb 2, 2026

kwonkwonn force-pushed the refactor/move-query-wsproxy-into-method branch 2 times, most recently from 646774b to 1469c3c Compare February 2, 2026 09:32

HyeockJinKim requested changes Feb 3, 2026

View reviewed changes

kwonkwonn requested a review from HyeockJinKim February 3, 2026 04:36

kwonkwonn force-pushed the refactor/move-query-wsproxy-into-method branch 2 times, most recently from e4d29d1 to 78b3f93 Compare February 3, 2026 07:44

kwonkwonn force-pushed the refactor/move-query-wsproxy-into-method branch from 78b3f93 to 992dc90 Compare February 3, 2026 07:50

github-actions bot added size:L 100~500 LoC comp:common Related to Common component and removed size:M 30~100 LoC labels Feb 3, 2026

fregataa previously approved these changes Feb 5, 2026

View reviewed changes

kwonkwonn dismissed fregataa’s stale review via 43a49ad February 5, 2026 04:56

kwonkwonn force-pushed the refactor/move-query-wsproxy-into-method branch from 992dc90 to 43a49ad Compare February 5, 2026 04:56

kwonkwonn commented Feb 5, 2026

View reviewed changes

kwonkwonn force-pushed the refactor/move-query-wsproxy-into-method branch from 43a49ad to 15c028b Compare February 5, 2026 05:13

github-actions bot added the comp:app-proxy Related to App Proxy component label Feb 5, 2026

kwonkwonn requested a review from fregataa February 5, 2026 05:26

kwonkwonn added 4 commits February 5, 2026 14:27

refactor: move out of convention function into valid directory

9b6fce8

refactor: move non-relevant function from the class

69d082f

refactor: move AppProxyClient pool function from legacy AgentRegistry

9243aa4

refactor: modify error handling, type consolidating, degrading legacy v1

65618a5

kwonkwonn force-pushed the refactor/move-query-wsproxy-into-method branch from 15c028b to 65618a5 Compare February 5, 2026 05:28

HyeockJinKim reviewed Feb 5, 2026

View reviewed changes

HyeockJinKim approved these changes Feb 5, 2026

View reviewed changes

HyeockJinKim added this pull request to the merge queue Feb 5, 2026

Merged via the queue into main with commit a8a49cd Feb 5, 2026
33 checks passed

HyeockJinKim deleted the refactor/move-query-wsproxy-into-method branch February 5, 2026 09:29

Conversation

kwonkwonn commented Feb 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kwonkwonn Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kwonkwonn commented Feb 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kwonkwonn commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Note

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kwonkwonn Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyeockJinKim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kwonkwonn Feb 2, 2026 •

edited

Loading

kwonkwonn commented Feb 3, 2026 •

edited

Loading

kwonkwonn Feb 5, 2026 •

edited

Loading