-
Notifications
You must be signed in to change notification settings - Fork 32
✨ New CatalogService in api-server that connects via rpc to the catalog micro-service
#7439
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
✨ New CatalogService in api-server that connects via rpc to the catalog micro-service
#7439
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7439 +/- ##
==========================================
- Coverage 87.45% 87.44% -0.01%
==========================================
Files 1717 1711 -6
Lines 66614 66493 -121
Branches 1132 1132
==========================================
- Hits 58259 58147 -112
+ Misses 8034 8025 -9
Partials 321 321
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
bbfe7e6 to
6df7f75
Compare
CatalogService in api-server that connects via rpc to the catalog micro-service
sanderegg
left a comment
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.
cool. but please consider my questions.
| **RESPONSE_MODEL_POLICY, | ||
| deprecated=True, | ||
| description="Use instead rpc._service.list_services_paginated -> PageRpcServicesGetV2", | ||
| ) |
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.
Q1: how are we going to handle "cancel_on_disconnect" with RPC calls?
Q2: I really want to move to faststream, for documentation purposes, and it seems they also have RPC (over Redis). but still I am wondering about Q1. cause we currently have the issue that we do not check connection status.
Currently the server side of the RPC will not cancel a call where the client timed-out or closed the connection for whatever reason.
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.
actually faststream also allows RPC over rabbitmq
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.
This RPC in fast stream is actually new. It was not there when I last checked
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.
From what I've gathered on the rabbitmq documentation page there is no guarantee of cancellation on remote (server side) by design. https://www.rabbitmq.com/docs/direct-reply-to#limitations
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.
you'd have to check if faststream implemented any support for it.
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.
actually faststream also allows RPC over rabbitmq
I tried this out at some point. It seemed quite easy to use. But (at least at the time I tried it) it didn't support (de)serialization via type-hints fastapi-style.
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.
Q1: how are we going to handle "cancel_on_disconnect" with RPC calls?
Good point.
If this is not implemented in the rpc-client by default, I am indeed not taking care of that.
Nonehtless, all the rpc operations in this PR are reading operations. If client disconnects, it is indeed not optimal but it is not dramatic. It might be a bigger problem if they are write operations. Moreover, in the case of cancel_on_disconnect is enabled, write operations also must guarantee some "atomicity" as well e.g. using units-of-work, to avoid inconsistent states.
Q2: I really want to move to faststream, for documentation purposes, and it seems they also have RPC (over Redis). but still I am wondering about Q1. cause we currently have the issue that we do not check connection status.
that would be great! I am all ears ... happy to hear your thoughts about it offline
Currently the server side of the RPC will not cancel a call where the client timed-out or closed the connection for whatever reason.
packages/models-library/src/models_library/api_schemas_catalog/services.py
Show resolved
Hide resolved
| **RESPONSE_MODEL_POLICY, | ||
| deprecated=True, | ||
| description="Use instead rpc._service.list_services_paginated -> PageRpcServicesGetV2", | ||
| ) |
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.
This RPC in fast stream is actually new. It was not there when I last checked
| **RESPONSE_MODEL_POLICY, | ||
| deprecated=True, | ||
| description="Use instead rpc._service.list_services_paginated -> PageRpcServicesGetV2", | ||
| ) |
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.
From what I've gathered on the rabbitmq documentation page there is no guarantee of cancellation on remote (server side) by design. https://www.rabbitmq.com/docs/direct-reply-to#limitations
| **RESPONSE_MODEL_POLICY, | ||
| deprecated=True, | ||
| description="Use instead rpc._service.list_services_paginated -> PageRpcServicesGetV2", | ||
| ) |
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.
you'd have to check if faststream implemented any support for it.
bisgaard-itis
left a comment
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.
Cool! Thanks a lot. Thanks also for not using the new catalog client yet in the api-server. That would have caused a lot of conflicts with what I am doing for the studies with associated data.
packages/pytest-simcore/src/pytest_simcore/helpers/catalog_rpc_server.py
Show resolved
Hide resolved
packages/service-library/src/servicelib/rabbitmq/rpc_interfaces/catalog/services.py
Show resolved
Hide resolved
services/api-server/src/simcore_service_api_server/services_rpc/catalog.py
Show resolved
Hide resolved
|
|
||
|
|
||
| @pytest.fixture | ||
| def mocked_rpc_catalog_service_api(mocker: MockerFixture) -> dict[str, MockType]: |
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.
Would it be possible to add this in pytest-simcore?
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.
Well not really because it mocks an object in the simcore_service_api_server
That said, that object is the catalog_rpc client that is in servicelib. Nonetheless, when it comes to mocks, the "path of import" is very important, even if you are mocking the same object. That is, i am not 100% sure that it would work if I patch the object importing instead from servicelib
We can check at some other point.
| **RESPONSE_MODEL_POLICY, | ||
| deprecated=True, | ||
| description="Use instead rpc._service.list_services_paginated -> PageRpcServicesGetV2", | ||
| ) |
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.
actually faststream also allows RPC over rabbitmq
I tried this out at some point. It seemed quite easy to use. But (at least at the time I tried it) it didn't support (de)serialization via type-hints fastapi-style.
3d32b27 to
1b60257
Compare
|




What do these changes do?
The
api-serverpreviously used REST to communicate with thecatalog, but those connections are now deprecated. A newCatalogServicehas been added to theapi-server, built on thecatalog_rpcinterface.Currently,
CatalogServiceis only used in tests. It will be adopted in the main code in a future PR, at which point the web clients will be removed.Details of the changes are as follows:
api-servermicro-serviceCatalogServiceto represent the "service layer" that interacts with thecatalogmicro-serviceservicelib.rabbitmq.rpc_interfaces.catalog.services.pyfor commsSolver,Program?CatalogServicein the rest-handlerscatalogmicro-serviceget_service_release_history(key) -> list[ServiceRelease]servicesRelated issue/s
How to test
Dev-ops
None