Skip to content

♻️ Refactoring of APIs for computations in web-server, api-server and directorv2 #7520

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 31 commits into from
Apr 15, 2025

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Apr 14, 2025

What do these changes do?

Cleanup of APIs related to computations in the web-api (web-server rest API), director-v2 (rest API) and finally the public-api (api-server's rest API).

web-api (web-server):

ReDoc

Some refactoring in thedirector-v2 domain in the web-server which is in charge of connecting with the directorv2 micro-service (i.e. services/web/server/src/simcore_service_webserver/director_v2)

  • refactor skeleton to include controler/service/repository (none) layers
src/simcore_service_webserver/director_v2
├── _controller
│   ├── __init__.py
│   ├── _rest_exceptions.py  # exception handling for rest
│   └── rest.py              # rest handlers
├── __init__.py
├── _client_base.py          
├── _client.py               # http client to the directorv-2 micro-service rest API
├── _service_abc_default.py  # default implementation of that ABC 
├── _service_abc.py          # service ABC to change run policy
├── _service.py              # service layer interface (private)
├── director_v2_service.py   # service layer interface (public) exposed to other domains
├── plugin.py
├── exceptions.py
└── settings.py
  • ♻️ /_handlers.py -> renamed /_contoller/rest.py
    • moved all schemas to packages/models-library/src/models_library/api_schemas_webserver/computations.py
      • For now we avoid enforcing camelcase
    • using parse_request_* functions
    • moved all exception handling to _rest_exceptions
  • ♻️ cleanup dependencies and api-specs in api/specs/web-server/_computations.py

directorv2

ReDoc

  • docs better rest API entrypoints for the /computations collection

public-api (api-server)

  • uses directorv2 schema on client
  • drops unused functions

Related issue/s

How to test

tests in place

Dev-ops

@pcrespov pcrespov self-assigned this Apr 14, 2025
Copy link

codecov bot commented Apr 14, 2025

Codecov Report

Attention: Patch coverage is 91.34948% with 25 lines in your changes missing coverage. Please review.

Project coverage is 88.16%. Comparing base (aac0abd) to head (ae4df2a).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7520      +/-   ##
==========================================
+ Coverage   87.44%   88.16%   +0.71%     
==========================================
  Files        1740     1725      -15     
  Lines       67432    66412    -1020     
  Branches     1144     1144              
==========================================
- Hits        58968    58554     -414     
+ Misses       8143     7537     -606     
  Partials      321      321              
Flag Coverage Δ
integrationtests 65.24% <84.43%> (-0.04%) ⬇️
unittests 86.66% <89.96%> (+0.04%) ⬆️
Components Coverage Δ
api 76.84% <ø> (ø)
pkg_aws_library 93.91% <ø> (ø)
pkg_dask_task_models_library 97.09% <ø> (ø)
pkg_models_library 91.98% <93.75%> (+0.02%) ⬆️
pkg_notifications_library 85.26% <ø> (ø)
pkg_postgres_database 88.18% <ø> (ø)
pkg_service_integration 70.03% <ø> (ø)
pkg_service_library 72.65% <ø> (ø)
pkg_settings_library 90.79% <ø> (ø)
pkg_simcore_sdk 85.40% <ø> (ø)
agent 96.46% <ø> (ø)
api_server 90.26% <100.00%> (+0.23%) ⬆️
autoscaling 96.08% <ø> (ø)
catalog 92.52% <ø> (ø)
clusters_keeper 99.24% <ø> (ø)
dask_sidecar 91.29% <ø> (ø)
datcore_adapter 98.12% <ø> (ø)
director 76.78% <ø> (ø)
director_v2 91.30% <100.00%> (-0.13%) ⬇️
dynamic_scheduler 97.40% <ø> (ø)
dynamic_sidecar 90.16% <ø> (+0.04%) ⬆️
efs_guardian 89.79% <ø> (ø)
invitations 93.28% <ø> (ø)
payments 92.66% <ø> (ø)
resource_usage_tracker 89.29% <ø> (+0.16%) ⬆️
storage 87.59% <ø> (+0.07%) ⬆️
webclient ∅ <ø> (∅)
webserver 88.27% <89.95%> (+2.39%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8e880a...ae4df2a. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pcrespov pcrespov force-pushed the is7516/web-api-tasks branch 2 times, most recently from 65e3ac6 to f836f15 Compare April 14, 2025 16:33
@pcrespov pcrespov changed the title WIP: Is7516/web api tasks ♻️ Refactoring of APIs for computations in web-server, api-server and directorv2 Apr 14, 2025
@pcrespov pcrespov added this to the Pauwel Kwak milestone Apr 14, 2025
@pcrespov pcrespov added a:webserver issue related to the webserver service a:api framework api, data schemas, a:director-v2 issue related with the director-v2 service a:apiserver api-server service labels Apr 14, 2025
@pcrespov pcrespov marked this pull request as ready for review April 14, 2025 17:28
@pcrespov pcrespov enabled auto-merge (squash) April 14, 2025 17:28
Copy link
Collaborator

@matusdrobuliak66 matusdrobuliak66 left a comment

Choose a reason for hiding this comment

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

👍 Thanks

@pcrespov pcrespov force-pushed the is7516/web-api-tasks branch from 58c2741 to 5452a7f Compare April 15, 2025 09:40
@pcrespov pcrespov added the t:maintenance Some planned maintenance work label Apr 15, 2025
@pcrespov pcrespov requested a review from Copilot April 15, 2025 09:41
@pcrespov
Copy link
Member Author

@mergify queue

Copy link
Contributor

mergify bot commented Apr 15, 2025

queue

🟠 Waiting for conditions to match

  • -closed [📌 queue requirement]
  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • branch-protection-review-decision = APPROVED [🛡 GitHub branch protection]
      • any of: [🛡 GitHub branch protection]
        • check-neutral = check OAS' are up to date
        • check-skipped = check OAS' are up to date
        • check-success = check OAS' are up to date
      • #approved-reviews-by >= 2 [🛡 GitHub branch protection]
      • #approved-reviews-by>=2
      • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
      • #changes-requested-reviews-by=0
      • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
      • #review-threads-unresolved=0
      • -conflict
      • -draft
      • base=master
      • label!=🤖-do-not-merge
      • label=🤖-automerge
      • any of: [🛡 GitHub branch protection]
        • check-skipped = deploy to dockerhub
        • check-neutral = deploy to dockerhub
        • check-success = deploy to dockerhub
      • any of: [🛡 GitHub branch protection]
        • check-success = system-tests
        • check-neutral = system-tests
        • check-skipped = system-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = unit-tests
        • check-neutral = unit-tests
        • check-skipped = unit-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = integration-tests
        • check-neutral = integration-tests
        • check-skipped = integration-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = [build] docker images (excluding frontend) (3.11, ubuntu-24.04)
        • check-neutral = [build] docker images (excluding frontend) (3.11, ubuntu-24.04)
        • check-skipped = [build] docker images (excluding frontend) (3.11, ubuntu-24.04)
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed

@pcrespov pcrespov added the 🤖-automerge marks PR as ready to be merged for Mergify label Apr 15, 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.

Copilot reviewed 42 out of 43 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • services/director-v2/openapi.json: Language not supported
Comments suppressed due to low confidence (1)

services/api-server/src/simcore_service_api_server/services_http/director_v2.py:88

  • [nitpick] The nested usage of model validation (wrapping DirectorV2ComputationGet.model_validate_json(response.text) inside ComputationTaskGet.model_validate) could be simplified to improve clarity. Consider refactoring to directly use a single validation step with the proper flags if supported.
return ComputationTaskGet.model_validate(

@pcrespov pcrespov enabled auto-merge (squash) April 15, 2025 11:30
@pcrespov pcrespov disabled auto-merge April 15, 2025 11:45
@pcrespov pcrespov merged commit 41523e7 into ITISFoundation:master Apr 15, 2025
2 of 4 checks passed
@pcrespov pcrespov deleted the is7516/web-api-tasks branch April 15, 2025 12:49
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖-automerge marks PR as ready to be merged for Mergify a:api framework api, data schemas, a:apiserver api-server service a:director-v2 issue related with the director-v2 service a:webserver issue related to the webserver service t:maintenance Some planned maintenance work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants