Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR converts the BaseService contract to an async interface (health() / info()), propagates that change across service implementations and key call sites (CLI/GUI), and adds a new FastAPI DI helper (BaseService.get_service()) plus a settings accessor.
Changes:
- Make
BaseService.health()/BaseService.info()async and update all service implementations accordingly. - Update system CLI/GUI and platform tests to await or run async health/info calls.
- Add
BaseService.get_service()(FastAPI dependency factory),BaseService.settings(), and accompanying tests/docs.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/aignostics/utils/service_test.py | Adds tests for BaseService.get_service() caching/behavior and settings() accessor. |
| tests/aignostics/platform/service_test.py | Updates platform service health test to async def + await. |
| src/aignostics/utils/_service.py | Makes base API async; adds get_service() and settings(). |
| src/aignostics/utils/CLAUDE.md | Updates docs/examples for async services and DI helper. |
| src/aignostics/platform/_service.py | Converts platform service health/info to async signature. |
| src/aignostics/system/_service.py | Converts system service health_static/health/info to async and awaits discovered services. |
| src/aignostics/system/_gui.py | Switches GUI health/info loading to await the async service methods. |
| src/aignostics/system/_cli.py | Runs async health/info via asyncio.run(...) in CLI commands. |
| src/aignostics/application/_service.py | Converts application service health/info to async signature. |
| src/aignostics/application/_cli.py | Runs async system health check via asyncio.run(...) before submitting runs. |
| src/aignostics/application/_gui/_page_application_describe.py | Updates GUI to call async system health (currently via asyncio.run). |
| src/aignostics/bucket/_service.py | Converts bucket service health/info to async signature. |
| src/aignostics/dataset/_service.py | Converts dataset service health/info to async signature. |
| src/aignostics/notebook/_service.py | Converts notebook service health/info to async signature. |
| src/aignostics/qupath/_service.py | Converts QuPath service health/info to async signature. |
| src/aignostics/wsi/_service.py | Converts WSI service health/info to async signature. |
| src/aignostics/CLAUDE.md | Updates root docs’ service examples to async signatures. |
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
olivermeyer
force-pushed
the
feat/bridge-baseservice
branch
from
b8c68d2 to
67cf115
Compare
olivermeyer
force-pushed
the
feat/bridge-baseservice
branch
from
67cf115 to
b17bd91
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the SDK’s service abstraction (BaseService) to use async health() / info() methods, and propagates that change through service implementations and key call sites (system aggregation, CLI, and GUI). It also adds a FastAPI dependency helper (get_service()) and a public settings accessor.
Changes:
- Make
BaseService.health()/BaseService.info()async and update allBaseServicesubclasses accordingly. - Add
BaseService.get_service()(cached FastAPI dependency factory) andBaseService.settings()accessor. - Update system CLI/GUI and selected GUI pages to call async health/info, and add unit tests for the new BaseService helpers.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/aignostics/utils/service_test.py | Adds unit coverage for BaseService.get_service() caching/DI behavior and settings(). |
| tests/aignostics/platform/service_test.py | Updates platform service health test to await the new async API. |
| src/aignostics/utils/_service.py | Converts abstract methods to async; adds get_service() DI helper and settings() accessor. |
| src/aignostics/utils/CLAUDE.md | Updates utils module docs/examples for async service methods and new helpers. |
| src/aignostics/CLAUDE.md | Updates top-level docs/examples for async service methods. |
| src/aignostics/application/_service.py | Converts service health() / info() to async. |
| src/aignostics/bucket/_service.py | Converts service health() / info() to async. |
| src/aignostics/dataset/_service.py | Converts service health() / info() to async. |
| src/aignostics/notebook/_service.py | Converts service health() / info() to async. |
| src/aignostics/platform/_service.py | Converts service health() / info() to async. |
| src/aignostics/qupath/_service.py | Converts service health() / info() to async. |
| src/aignostics/wsi/_service.py | Converts service health() / info() to async. |
| src/aignostics/system/_service.py | Makes system aggregation APIs async and awaits discovered services’ health/info. |
| src/aignostics/system/_cli.py | Bridges async health/info via asyncio.run() for Typer CLI commands. |
| src/aignostics/system/_gui.py | Updates system GUI page to await async health/info. |
| src/aignostics/gui/_frame.py | Updates launchpad health polling to await async system health. |
| src/aignostics/application/_cli.py | Bridges async system health check via asyncio.run() in CLI. |
| src/aignostics/application/_gui/_page_application_describe.py | Updates application describe page to await async system health. |
Collaborator
Author
|
One reply to all Copilot comments above: making all calls async is out of scope here, I just want to unify the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
olivermeyer
requested a review
from helmut-hoffer-von-ankershoffen
as a code owner
olivermeyer
added
claude
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR aligns the SDK’s BaseService interface with Bridge by making health()/info() async, adding a FastAPI-friendly dependency factory (get_service) plus a settings accessor (settings), and updating service callers across CLI/GUI to accommodate the async contract.
Changes:
- Make
BaseService.health()andBaseService.info()asyncand update concrete service implementations accordingly. - Add
BaseService.get_service()(cached FastAPI dependency factory) andBaseService.settings()accessor, plus new unit tests. - Update CLI/GUI call sites to use
await/asyncio.run(...)for async health/info retrieval.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/aignostics/utils/service_test.py | Adds unit tests for get_service() caching/behavior and settings(), plus coroutine assertions for health/info. |
| tests/aignostics/platform/service_test.py | Updates the platform service health test to await the async health(). |
| src/aignostics/wsi/_service.py | Converts health()/info() to async to match BaseService. |
| src/aignostics/utils/_service.py | Introduces get_service() and settings(), and makes health()/info() async abstract methods. |
| src/aignostics/utils/CLAUDE.md | Updates service examples/docs to reflect async methods and new DI/settings helpers. |
| src/aignostics/system/_service.py | Makes health_static, health, and info async and updates internal aggregation calls to await. |
| src/aignostics/system/_gui.py | Updates GUI to await async health/info calls (removes previous run.cpu_bound usage). |
| src/aignostics/system/_cli.py | Wraps async service calls in asyncio.run() for CLI execution. |
| src/aignostics/qupath/_service.py | Converts health()/info() to async. |
| src/aignostics/platform/_service.py | Converts health()/info() to async. |
| src/aignostics/notebook/_service.py | Converts health()/info() to async. |
| src/aignostics/gui/_frame.py | Updates periodic health polling to await async SystemService.health_static(). |
| src/aignostics/dataset/_service.py | Converts health()/info() to async. |
| src/aignostics/bucket/_service.py | Converts health()/info() to async. |
| src/aignostics/application/_service.py | Converts health()/info() to async. |
| src/aignostics/application/_gui/_page_application_describe.py | Updates GUI flow to await system health check once during page load. |
| src/aignostics/application/_cli.py | Uses asyncio.run(SystemService.health_static()) in the sync CLI path. |
| src/aignostics/CLAUDE.md | Updates top-level docs to reflect async service method signatures. |
Comments suppressed due to low confidence (1)
src/aignostics/system/_gui.py:91
await Service.info(...)does blocking system calls (psutil.cpu_percent(interval=...), etc.) insideService.info. Running it directly on the NiceGUI event loop can stall the UI during the measurement interval. Consider running info collection in a worker thread (nicegui.run.io_bound/cpu_bound) or refactor the implementation to avoid blocking calls in the async path.
editor.set_visibility(False)
spinner.set_visibility(True)
mask_secrets_switch.set_visibility(False)
info = await Service.info(include_environ=True, mask_secrets=mask_secrets)
if info is None:
properties["content"] = {"json": "Info retrieval failed."} # type: ignore[unreachable]
else:
properties["content"] = {"json": info}
This comment was marked as outdated.
This comment was marked as outdated.
olivermeyer
force-pushed
the
feat/bridge-baseservice
branch
from
e79128c to
0ec2bca
Compare
olivermeyer
force-pushed
the
feat/bridge-baseservice
branch
from
0ec2bca to
6c6b15c
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the SDK’s service abstraction to better align with Bridge by making BaseService.health() / BaseService.info() async, adding a FastAPI DI helper (get_service) and a settings() accessor, and propagating the async interface through services plus CLI/GUI call sites.
Changes:
- Add
BaseService.get_service()(FastAPI dependency factory) andBaseService.settings() - Make
BaseService.health()/info()async and update concrete services + call sites (CLI/GUI) - Add/adjust tests and docs to reflect the new async service interface
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/aignostics/utils/service_test.py | New unit tests for get_service() caching/behavior and settings(); coroutine checks for async methods |
| tests/aignostics/platform/service_test.py | Update platform service unit test to await async health() |
| src/aignostics/utils/_service.py | Introduce get_service() + settings(); convert abstract health/info to async |
| src/aignostics/utils/CLAUDE.md | Update docs/examples for async health/info and new DI/settings APIs |
| src/aignostics/system/_service.py | Convert system health/info entrypoints to async and await discovered services |
| src/aignostics/system/_gui.py | Update GUI to await async system health/info entrypoints |
| src/aignostics/system/_cli.py | Wrap async system health/info calls with asyncio.run() for sync CLI commands |
| src/aignostics/application/_service.py | Convert service health/info to async |
| src/aignostics/application/_cli.py | Use asyncio.run() to call async system health check from sync CLI preflight |
| src/aignostics/application/_gui/_page_application_describe.py | Await async system health check for GUI flow decisions |
| src/aignostics/platform/_service.py | Convert platform service health/info to async |
| src/aignostics/bucket/_service.py | Convert bucket service health/info to async |
| src/aignostics/dataset/_service.py | Convert dataset service health/info to async |
| src/aignostics/wsi/_service.py | Convert WSI service health/info to async |
| src/aignostics/qupath/_service.py | Convert QuPath service health/info to async |
| src/aignostics/notebook/_service.py | Convert notebook service health/info to async |
| src/aignostics/gui/_frame.py | Update frame health polling to await async system health check |
| src/aignostics/CLAUDE.md | Update top-level docs/examples for async health/info |
| .github/workflows/_test.yml | Rename failure labels for e2e variants in the workflow summary |
Comments suppressed due to low confidence (2)
src/aignostics/platform/_service.py:186
info()is nowasyncbut has noawait, which will trigger RuffRUF029and fail linting. Either add# noqa: RUF029on the definition or offload the synchronous work (token/userinfo retrieval) via an awaited async boundary (e.g.,asyncio.to_thread).
async def info(self, mask_secrets: bool = True) -> dict[str, Any]:
"""Determine info of this service.
Args:
mask_secrets (bool): Whether to mask sensitive information in the output.
Returns:
dict[str,Any]: The info of this service.
"""
user_info = None
try:
user_info = self.get_user_info()
except RuntimeError:
src/aignostics/platform/_service.py:293
health()is nowasyncbut has noawaitand still performs synchronous network I/O (urllib3.request(...)via_determine_api_*_health). This will both (1) be flagged by RuffRUF029and (2) block any running event loop when awaited. Consider offloading the blocking checks to a thread (which introduces anawait) or add# noqa: RUF029if you intentionally keep it synchronous for now.
async def health(self) -> Health:
"""Determine health of this service.
Returns:
Health: The health of the service.
"""
return Health(
status=Health.Code.UP,
components={
"api_public": self._determine_api_public_health(),
"api_authenticated": self._determine_api_authenticated_health(),
},
)
6c6b15c to
ff75bfc
Compare
olivermeyer
force-pushed
the
feat/bridge-baseservice
branch
from
ff75bfc to
3613113
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the SDK’s service abstraction to align BaseService with Bridge by adding FastAPI-friendly dependency injection helpers and converting the core health()/info() contract to async, then propagating those signature changes through services, CLI, GUI, and tests.
Changes:
- Added
BaseService.get_service()(FastAPI dependency factory) andBaseService.settings()accessor. - Made
BaseService.health()/BaseService.info()async and updated concrete service implementations + affected call sites (CLI/GUI/system aggregation). - Added/updated unit tests to cover the new service helpers and async behavior.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/aignostics/utils/service_test.py | New tests for BaseService.get_service() caching semantics, settings(), and async method shape. |
| tests/aignostics/platform/service_test.py | Updates a platform health test to async def and awaits Service.health(). |
| src/aignostics/utils/_service.py | Introduces get_service() + settings() and makes abstract health/info async. |
| src/aignostics/utils/CLAUDE.md | Updates documentation examples for async health/info and documents get_service() + settings(). |
| src/aignostics/wsi/_service.py | Updates service to async health/info to satisfy new BaseService contract. |
| src/aignostics/platform/_service.py | Updates platform service to async health/info. |
| src/aignostics/bucket/_service.py | Updates bucket service to async health/info. |
| src/aignostics/dataset/_service.py | Updates dataset service to async health/info. |
| src/aignostics/application/_service.py | Updates application service to async health/info. |
| src/aignostics/qupath/_service.py | Updates qupath service to async health/info. |
| src/aignostics/notebook/_service.py | Updates notebook service to async health/info. |
| src/aignostics/system/_service.py | Updates system aggregation/service methods (health_static, health, info) to async and awaits discovered services. |
| src/aignostics/system/_cli.py | Wraps async service calls via asyncio.run() in CLI commands. |
| src/aignostics/application/_cli.py | Wraps SystemService.health_static() via asyncio.run() for CLI preflight health check. |
| src/aignostics/system/_gui.py | Updates GUI to await async health_static() / info() directly. |
| src/aignostics/gui/_frame.py | Updates periodic health UI to await SystemService.health_static() directly. |
| src/aignostics/application/_gui/_page_application_describe.py | Updates application describe page to await SystemService.health_static() and uses the result in stepper logic. |
| src/aignostics/CLAUDE.md | Updates top-level docs examples to use async health/info. |
olivermeyer
force-pushed
the
feat/bridge-baseservice
branch
from
3613113 to
8279335
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the SDK’s service abstraction to better align with Bridge by making BaseService.health() / BaseService.info() asynchronous, adding a FastAPI DI helper (get_service) plus a settings() accessor, and updating CLI/GUI/tests to use the new async contract.
Changes:
- Make
BaseService.health()andBaseService.info()async, and update all concrete service implementations accordingly. - Add
BaseService.get_service()(cached FastAPI dependency factory) andBaseService.settings()accessor + unit tests. - Update CLI/GUI/test call sites to
awaitservice calls or wrap them viaasyncio.run(); migrate platform/system health checks fromurllib3tohttpxasync client.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/aignostics/utils/service_test.py | Adds unit tests for get_service() caching, settings(), and async coroutine behavior. |
| tests/aignostics/system/service_pooling_test.py | Removes pooling tests corresponding to removal of urllib3 pool usage. |
| tests/aignostics/system/cli_test.py | Updates mocks to AsyncMock for async health(). |
| tests/aignostics/platform/service_test.py | Converts tests to async and updates mocking for httpx.AsyncClient. |
| tests/aignostics/application/gui_test.py | Runs blocking CliRunner.invoke via asyncio.to_thread in async GUI tests. |
| src/aignostics/utils/_service.py | Adds get_service() + settings() and makes health()/info() async abstract methods. |
| src/aignostics/utils/CLAUDE.md | Updates docs/examples for async service methods + DI usage. |
| src/aignostics/system/_service.py | Makes health/info async, switches network calls to httpx, and avoids blocking CPU sampling by using asyncio.sleep. |
| src/aignostics/system/_gui.py | Updates GUI to await async health_static() / info(). |
| src/aignostics/system/_cli.py | Wraps async service calls with asyncio.run() for synchronous Typer commands. |
| src/aignostics/platform/_service.py | Migrates health checks from urllib3 to async httpx and makes service methods async. |
| src/aignostics/application/_service.py | Makes health() / info() async. |
| src/aignostics/application/_gui/_page_application_describe.py | Updates system health usage to async/await. |
| src/aignostics/application/_cli.py | Wraps async system health check with asyncio.run(). |
| src/aignostics/bucket/_service.py | Makes health() / info() async. |
| src/aignostics/dataset/_service.py | Makes health() / info() async. |
| src/aignostics/wsi/_service.py | Makes health() / info() async. |
| src/aignostics/qupath/_service.py | Makes health() / info() async. |
| src/aignostics/notebook/_service.py | Makes health() / info() async. |
| src/aignostics/gui/_frame.py | Updates periodic health check to await async SystemService.health_static(). |
| src/aignostics/CLAUDE.md | Updates top-level docs to reflect async service API. |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… checks Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Extract CPU frequency measurement into a dedicated private static method to reduce cognitive complexity of Service.info() from 17 to 10. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace 9 inline occurrences of the patch target string "aignostics.platform._service.httpx.AsyncClient" with a single module-level constant _PATCH_HTTPX_ASYNC_CLIENT, following the existing _PATCH_AUTH_GETTER convention. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
olivermeyer
force-pushed
the
feat/bridge-baseservice
branch
from
f1d0f72 to
dcc8028
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Unifying the
BaseServicewith Bridge.The main changes are:
get_servicemethod for FastAPI injection and asettingsmethodhealthandinfomethods async + adjusting concrete servicesasyncio.run()healthandinfoI am not making all methods on the various service implementations async.