-
Notifications
You must be signed in to change notification settings - Fork 29
♻️ Use catalog rpc client in api-server #7541
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
♻️ Use catalog rpc client in api-server #7541
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7541 +/- ##
==========================================
+ Coverage 87.49% 87.56% +0.06%
==========================================
Files 1743 1738 -5
Lines 67497 67364 -133
Branches 1144 1144
==========================================
- Hits 59054 58984 -70
+ Misses 8122 8059 -63
Partials 321 321
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nice work! I specially enjoyed the technique of using dependencies in the constructor.
Left some suggestions for your consideration
services/web/server/src/simcore_service_webserver/api/v0/openapi.yaml
Outdated
Show resolved
Hide resolved
packages/pytest-simcore/src/pytest_simcore/helpers/catalog_rpc_server.py
Show resolved
Hide resolved
services/api-server/src/simcore_service_api_server/_service_job.py
Outdated
Show resolved
Hide resolved
services/api-server/src/simcore_service_api_server/_service_job.py
Outdated
Show resolved
Hide resolved
services/api-server/src/simcore_service_api_server/api/routes/programs.py
Outdated
Show resolved
Hide resolved
services/api-server/src/simcore_service_api_server/api/routes/programs.py
Outdated
Show resolved
Hide resolved
services/api-server/src/simcore_service_api_server/models/schemas/solvers.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot!
THOUGHT: @bisgaard-itis @pcrespov - From a design perspective, I think I’d prefer if all the logic were built into the CORE service rather than the API server.
Reason? Reusability.
Right now, you’re introducing a new concept "programs" from the perspective of the public API, but eventually, some UI elements will be needed (btw, even now, just creating an API key requires going through the UI — so these things are coupled).
I believe the API gateway or web API should act purely as proxies/gateways to the CORE application - which currently lives in the webserver container. This core service owns almost everything important: projects, users, access rights, wallets, etc.
I see your point. I would argue though, that the issue is that we are coupling concepts in the webserver and the api-server. I don't think that's the way to go. We don't need to do this coupling except maybe for some technical details (like the new |
@bisgaard-itis Yes, I see your point 👍 As always, both approaches have their pros and cons. :) |
|
What do these changes do?
SolverService
,ProgramsService
andJobService
in the api-server. These parts of the service layer in the api-server are new in the sense that they build on top of the "raw" clients to the backend. They make use of the FastAPI dependency injection system for handling the their dependency on the underlyingCatalogService
. This reduces boilerplate code and simplifies the app initialization phase.Related issue/s
How to test
Dev-ops checklist