INTERSECT-SDK / python-sdk

Interconnected Science Ecosystem - Software Development Kit (INTERSECT-SDK)
https://intersect-python-sdk.readthedocs.io
BSD 3-Clause "New" or "Revised" License
4 stars 1 forks source link

Multi-capability and svc2svc support for services #9

Closed Lance-Drane closed 3 months ago

Lance-Drane commented 3 months ago

From @MichaelBrim:

This changeset includes updates to enable each IntersectService instance to provide service for a set of capabilities, rather than just one. From the client perspective, the key difference is that operation names should be prepended with the advertised name of the capability providing the operation.

Also included is support for service-to-service requests, as is necessary to enable making external requests to other services/capabilities as part of the implementation of any service methods. An additional thread is used to process these external requests asynchronously from incoming request handlers.

gecage952 commented 3 months ago

So basically services use create_external_request to just send messages to other services? Seems nice. Would it make sense to rename IntersectClientMessageParams since it's not exclusively being used by clients anymore? Maybe, just remove the client part.

gecage952 commented 3 months ago

Overall, looks good to me as well with only a couple of pretty minor comments.

Lance-Drane commented 3 months ago

So basically services use create_external_request to just send messages to other services? Seems nice.

Yep! Added this in one of the commits today. The API may be a little awkward since it returns a list of UUIDs instead of one UUID, but in practice you should be able to just reference [0] (and that's if you even need the UUID in your Capability, I don't think you do)

Would it make sense to rename IntersectClientMessageParams since it's not exclusively being used by clients anymore? Maybe, just remove the client part.

I ended up renaming this to IntersectDirectMessageParams to draw a distinction between an explicit Service object and an explicit Client object. Additionally, I reorganized the callback modules into client_callback_definitions, service_callback_definitions, and shared_callback_definitions to more explicitly organize these between Client and Service - IntersectDirectMessageParams is in the shared module.

gecage952 commented 3 months ago

This all looks good to me.

marshallmcdonnell commented 3 months ago

Just to check, @MichaelBrim, are you good with this now?

I think once we have your approval in, we are good to go with a merge!

Mainly checking in since this is a current blocker for @gecage952

Lance-Drane commented 3 months ago

I'd ideally like to have an E2E test in but we can merge this and cut an early 0.7.0 release.

Lance-Drane commented 3 months ago

@MichaelBrim or @gecage952 do either of you have the time to create an E2E test this week? I would prefer @MichaelBrim do it but he may not have the capacity.

The E2E test would involve:

1) Creating an example in the examples section which explicitly uses the new create_external_request function call in a service "A" to call another service "B" and get a result back from them. The client should only talk to service "A" directly. This value should be consistent across tests. 2) The client should print this value from the service to stdout, all other logging should go to stderr 3) editing tests/e2e/test_examples.py to add an E2E test like this:

def test_example_4_service_to_service():
    assert run_example_test('4_service_to_service') == 'Your output here\n'

You need to make sure that the directory containing the example has the same name as the argument to run_example_test, that all service executables end with _service.py (and nothing but service executables end with _service.py), and that the client ends with _client.py (and only the client ends with _client.py).

A docs page would also be nice, but is not essential at this time. If you copy how the other examples are being done, the only thing you'd need to modify would be the output, unless you choose to have Service A call Service B on startup instead of inside an endpoint.

MichaelBrim commented 3 months ago

@Lance-Drane I can probably make some time to do that this week, but no promises. I'm also on travel next week.

Lance-Drane commented 3 months ago

@MichaelBrim thanks, if you're only able to partially complete it just push it to a branch and myself, Greg, or Marshall can take a look at it.

marshallmcdonnell commented 3 months ago

From conversations, @gecage952 will take a look at this for adding an example (thank you, Greg :+1: )

Lance-Drane commented 3 months ago

OK, E2E test is in - had to fix a minor issue with how the service was cleaning up external requests but otherwise everything looks good. Thank you to both @gecage952 and @MichaelBrim for the contributions! I will merge this and release v0.7.0 shortly.