-
Notifications
You must be signed in to change notification settings - Fork 4
External callbacks via stomp #1535
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1535 +/- ##
==========================================
+ Coverage 92.65% 92.72% +0.06%
==========================================
Files 151 152 +1
Lines 8433 8499 +66
==========================================
+ Hits 7814 7881 +67
+ Misses 619 618 -1
🚀 New features to boost your workflow:
|
60a3bdd to
e80df89
Compare
8b272ee to
4cdf50e
Compare
DominicOram
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.
This looks good but I haven't had a chance to run the system tests. Couple of comments in code but mostly future looking stuff. Additionally, the config says we're trying to connect to stomp at tcp://localhost:61613, do we have to run this up ourselves? Or is this the GDA activeMQ that is already running? Can we add the answer to https://diamondlightsource.github.io/mx-bluesky/main/developer/hyperion/hyperion-blueapi.html?
| BLUEAPI_EVENT_TOPIC = "public.worker.event" | ||
|
|
||
|
|
||
| class StompDispatcher(Dispatcher): |
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.
Could: This seems generic enough that it should live in stomp-bluesky can you make a PR in there for it? Or at least an issue
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.
There is an outstanding issue DiamondLightSource/bluesky-stomp#14 - I've referenced this PR in it.
Unfortunately this file would introduce a circular dependency between stomp-bluesky and blueapi if introduced there.
|
|
||
| from mx_bluesky.common.utils.log import ISPYB_ZOCALO_CALLBACK_LOGGER as LOGGER | ||
|
|
||
| BLUEAPI_EVENT_TOPIC = "public.worker.event" |
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.
Should: Feels like we should get this out of blueapi somewhere, can you add an issue/PR to expose it in blueapi?
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.
DominicOram
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.
Thanks, I still think this needs the docs I suggested though
This addresses the hyperion portion of
This requires:
Link to dodal PR (if required): #N/A
(remember to update
pyproject.tomlwith the dodal commit tag if you need it for tests to pass!)--stomp-configparameter specifying a blueapi config file which contains the stomp configurationInstructions to reviewer on how to test:
hyperion-system-testChecks for reviewer