Skip to content
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

Multi-capability and svc2svc support for services #9

Merged
merged 10 commits into from
Aug 21, 2024
Merged

Conversation

Lance-Drane
Copy link
Collaborator

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.

Copy link
Collaborator Author

@Lance-Drane Lance-Drane left a comment

Choose a reason for hiding this comment

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

Would like a quick review of these changes from @MichaelBrim but otherwise this MR is probably good to merge in once we add a basic E2E test which uses some of the service-to-service functionality.

There are a couple of other features we may want to add (based on @gecage952 needs) but these can be addressed in a separate MR:

  • Allow a service to listen for events from another service
  • Add an API to the IntersectBaseCapabilityImplementation class to allow for a capability to call the service's create_external_request function.

src/intersect_sdk/service.py Outdated Show resolved Hide resolved
src/intersect_sdk/service.py Outdated Show resolved Hide resolved
src/intersect_sdk/service.py Show resolved Hide resolved
src/intersect_sdk/service.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@MichaelBrim MichaelBrim left a comment

Choose a reason for hiding this comment

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

Most of the changes are good to go. I commented and resolved those conversations.

There is still one thing to be fixed. See my answer to question about IntersectService._process_external_request() response handling.

@gecage952
Copy link
Collaborator

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
Copy link
Collaborator

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

@Lance-Drane
Copy link
Collaborator Author

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
Copy link
Collaborator

This all looks good to me.

@marshallmcdonnell
Copy link
Collaborator

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

Copy link
Collaborator

@MichaelBrim MichaelBrim left a comment

Choose a reason for hiding this comment

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

Looks like the _ExternalRequest.got_valid_response addition addresses my only concern. Good to go from my perspective.

@Lance-Drane
Copy link
Collaborator Author

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
Copy link
Collaborator Author

@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
Copy link
Collaborator

@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
Copy link
Collaborator Author

@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
Copy link
Collaborator

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

@Lance-Drane
Copy link
Collaborator Author

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.

MichaelBrim and others added 10 commits August 21, 2024 12:21
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.
Signed-off-by: Lance Drane <dranelt@ornl.gov>
Signed-off-by: Lance Drane <dranelt@ornl.gov>
…ectClientMessageParams' to 'DirectMessageParams'

Signed-off-by: Lance Drane <dranelt@ornl.gov>
Signed-off-by: Lance Drane <dranelt@ornl.gov>
Signed-off-by: Lance Drane <dranelt@ornl.gov>
Signed-off-by: Lance Drane <dranelt@ornl.gov>
@Lance-Drane Lance-Drane merged commit dec19bb into main Aug 21, 2024
17 checks passed
@Lance-Drane Lance-Drane deleted the svc2svc branch August 21, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants