Skip to content

♻️ Refactor simcore_service_catalog: Apply Layered Architecture and Initialization Cleanup #7491

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 10, 2025

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Apr 8, 2025

What do these changes do?

This PR refactors the simcore_service_catalog service to align with a cleaner layered architecture (controller–service–repository). Key improvements include:

  • Adopting the new initialization/lifespan pattern introduced in PR 🎨 catalog: lifespan managers for fastapi apps #7483
  • Removing redundant boilerplate in error handling
  • Reorganizing the project structure to better reflect the responsibilities of each layer
  • Increase further test coverage and fixes some flaky tests

A commented version of the updated service skeleton is included for clarity.

src/simcore_service_catalog
├── api                             # controller
│   ├── __init__.py
│   ├── _dependencies           # coupling with service layer
│   │   ├── __init__.py
│   │   ├── database.py
│   │   ├── director.py
│   │   ├── services.py
│   │   └── user_groups.py
│   ├── rest
│   │   ├── __init__.py
│   │   ├── _health.py        # rest handlers ...      
│   │   ├── _meta.py
│   │   ├── _services_access_rights.py
│   │   ├── _services_extras.py
│   │   ├── _services_labels.py
│   │   ├── _services_ports.py
│   │   ├── _services.py
│   │   ├── _services_resources.py
│   │   └── _services_specifications.py
│   │   ├── routes.py         # setup for rest (adds routes and error handling)
│   │   ├── errors.py         # error handlers
│   └── rpc
│       ├── __init__.py
│       └── _services.py      # rpc handlers ...
│       ├── events.py         # lifespan/setup for rpc
│ 
├── clients                   # clients to other services  
│   ├── __init__.py
│   ├── director.py
│   └── rabbitmq.py   # a part of this will be moved to servicelib.fastapi.rabbit in coming PRs
├── core
│   ├── __init__.py
│   ├── application.py
│   ├── background_tasks.py
│   ├── events.py             # lifespan/setup for application  
│   └── settings.py
├── models                    # domain models   
│   ├── __init__.py
│   ├── services_db.py
│   └── services_specifications.py
├── repository                # repository layer
│   ├── __init__.py
│   ├── _base.py
│   ├── _services_sql.py
│   ├── events.py             # lifespan/setup for repo
│   ├── groups.py
│   ├── products.py
│   ├── projects.py
│   └── services.py
├── service                   # service layer
│   ├── __init__.py
│   ├── access_rights.py
│   ├── compatibility.py
│   ├── function_services.py
│   ├── manifest.py    
│   └── services.py     # unfortunately this is the name of the resource we handle in the catalog. Any suggestions?
├── utils
│   ├── __init__.py
│   ├── service_resources.py
│   └── versioning.py
├── __init__.py
├── _constants.py
├── _meta.py
├── errors.py                  # domain exceptions
├── cli.py                     # CLI (this is a mix of handlers (i.e. controller) and main app. Could be split as the rest/rpc at some point)    
└── main.py

Related issue/s

How to test

cd services/catalog
make install-dev
make test-dev-unit

Dev-ops

None

@pcrespov pcrespov added the a:catalog catalog service label Apr 8, 2025
@pcrespov pcrespov added this to the Pauwel Kwak milestone Apr 8, 2025
@pcrespov pcrespov self-assigned this Apr 8, 2025
Copy link

codecov bot commented Apr 8, 2025

Codecov Report

Attention: Patch coverage is 95.95376% with 7 lines in your changes missing coverage. Please review.

Project coverage is 87.32%. Comparing base (cc560d3) to head (83f406b).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7491      +/-   ##
==========================================
- Coverage   87.44%   87.32%   -0.13%     
==========================================
  Files        1741     1566     -175     
  Lines       67375    63372    -4003     
  Branches     1142      908     -234     
==========================================
- Hits        58919    55339    -3580     
+ Misses       8138     7774     -364     
+ Partials      318      259      -59     
Flag Coverage Δ
integrationtests 65.10% <ø> (-0.03%) ⬇️
unittests 86.46% <95.95%> (-0.18%) ⬇️
Components Coverage Δ
api ∅ <ø> (∅)
pkg_aws_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library 91.96% <100.00%> (+0.01%) ⬆️
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration 70.03% <ø> (ø)
pkg_service_library 72.80% <0.00%> (-0.06%) ⬇️
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 85.40% <ø> (ø)
agent 96.46% <ø> (ø)
api_server 90.02% <ø> (ø)
autoscaling 96.08% <ø> (ø)
catalog 92.53% <96.91%> (+0.61%) ⬆️
clusters_keeper 99.24% <ø> (ø)
dask_sidecar 91.29% <ø> (ø)
datcore_adapter 98.12% <ø> (ø)
director 76.78% <ø> (ø)
director_v2 91.27% <ø> (-0.03%) ⬇️
dynamic_scheduler 97.35% <ø> (ø)
dynamic_sidecar 90.11% <ø> (ø)
efs_guardian 89.79% <ø> (ø)
invitations 93.28% <ø> (ø)
payments 92.66% <ø> (ø)
resource_usage_tracker 89.18% <ø> (ø)
storage 87.84% <ø> (ø)
webclient ∅ <ø> (∅)
webserver 85.87% <ø> (-0.01%) ⬇️

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 cc560d3...83f406b. 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 mai/catalog-simplification branch 2 times, most recently from 5559d74 to 71052a2 Compare April 10, 2025 12:13
@pcrespov pcrespov changed the title ♻️ Mai/catalog simplification ♻️ Refactor simcore_service_catalog: Apply Layered Architecture and Initialization Cleanup Apr 10, 2025
@pcrespov pcrespov marked this pull request as ready for review April 10, 2025 15:48
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 66 out of 66 changed files in this pull request and generated no comments.

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.

👍 nice

@matusdrobuliak66
Copy link
Collaborator

matusdrobuliak66 commented Apr 10, 2025

It's a matter of style, but as you know, I'm a bigger proponent of organizing code by feature or domain (vertical slicing) rather than by type (horizontal slicing). Each domain or feature folder contains everything it needs to function :P

/user
  ├── controller.py
  ├── service.py
  ├── repository.py
  ├── models.py
  └── schema.py
/order
  ├── controller.py
  ├── service.py
  ├── repository.py
  ├── models.py
  └── schema.py

but anyway this organization on controller - service - repository pattern is very very good! thank you

Just for reference:

src/simcore_service_catalog
├── services/                             # domain: services
│   ├── __init__.py
│   ├── api/
│   │   ├── rest.py                      # includes _services.py + related routes
│   │   └── rpc.py                       # rpc handler for services
│   ├── service.py                       # business logic
│   ├── repository.py                    # db layer
│   ├── models.py                        # schemas/models for services
│   └── dependencies.py                  # api deps, e.g. get_service_by_id
│
├── service_labels/                      # domain: service labels
│   ├── __init__.py
│   ├── api/
│   │   └── rest.py                      # _services_labels.py
│   ├── service.py
│   ├── repository.py
│   └── models.py
│
├── service_resources/                   # domain: service resources (was utils/service_resources.py)
│   ├── __init__.py
│   ├── api/
│   │   └── rest.py                      # _services_resources.py
│   ├── service.py
│   └── repository.py
│
├── service_access_rights/
│   ├── __init__.py
│   ├── api/
│   │   └── rest.py                      # _services_access_rights.py
│   ├── service.py
│   └── repository.py
│
├── service_ports/
│   ├── __init__.py
│   ├── api/
│   │   └── rest.py                      # _services_ports.py
│   ├── service.py
│   └── models.py
│
├── service_specifications/
│   ├── __init__.py
│   ├── api/
│   │   └── rest.py                      # _services_specifications.py
│   ├── service.py
│   ├── repository.py
│   └── models.py                        # previously services_specifications.py
│
├── service_extras/
│   ├── __init__.py
│   ├── api/
│   │   └── rest.py                      # _services_extras.py
│   └── service.py
│
├── health/                               # domain: health check
│   ├── __init__.py
│   └── api/
│       └── rest.py                      # _health.py
│
├── core/                                 # app-wide stuff
│   ├── __init__.py
│   ├── application.py
│   ├── background_tasks.py
│   ├── events.py
│   ├── settings.py
│   └── constants.py                     # moved _constants.py
│
├── meta/                                 # app meta info
│   ├── __init__.py
│   └── api/
│       └── rest.py                      # _meta.py
│
├── clients/                              # external service clients
│   ├── __init__.py
│   ├── director.py
│   └── rabbitmq.py
│
├── errors/                               # shared errors across domains
│   ├── __init__.py
│   └── errors.py
│
├── main.py
├── cli.py                                # Could become cli/ with subcommands
└── __init__.py

@matusdrobuliak66
Copy link
Collaborator

services.py # unfortunately this is the name of the resource we handle in the catalog. Any suggestions? --> user_services?

Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

not sure I get the singular or plural of the folders.
I suggest using simcore_services.py or user_services.py

@pcrespov pcrespov enabled auto-merge (squash) April 10, 2025 17:47
@pcrespov pcrespov added the 🤖-automerge marks PR as ready to be merged for Mergify label Apr 10, 2025
@pcrespov
Copy link
Member Author

@mergify queue

Copy link
Contributor

mergify bot commented Apr 10, 2025

queue

🟠 Waiting for conditions to match

  • -closed [📌 queue requirement]
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed
  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • #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
      • branch-protection-review-decision = APPROVED [🛡 GitHub branch protection]
      • 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 = check OAS' are up to date
        • check-neutral = check OAS' are up to date
        • check-skipped = check OAS' are up to date
      • 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)

Copy link
Contributor

@giancarloromeo giancarloromeo left a comment

Choose a reason for hiding this comment

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

Thanks a lot

pcrespov added 20 commits April 10, 2025 21:49
…with repository_lifespan for improved clarity
…led to background_task_lifespan_disabled for improved clarity
… repository_lifespan_manager for improved clarity
@pcrespov pcrespov force-pushed the mai/catalog-simplification branch from 21d13a5 to 12cc2d3 Compare April 10, 2025 19:49
Copy link

@pcrespov pcrespov merged commit 01c090f into ITISFoundation:master Apr 10, 2025
94 checks passed
@pcrespov pcrespov deleted the mai/catalog-simplification branch April 11, 2025 07:23
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:catalog catalog service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants