-
Notifications
You must be signed in to change notification settings - Fork 2
add new arroyo listener #16
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
add new arroyo listener #16
Conversation
| self.operator = operator | ||
| self.beamline_runs_tiled = beamline_runs_tiled | ||
| self.tiled_frame_segments = tiled_frame_segments | ||
| self.poll_pause_sec = poll_pause_sec # Not used, but kept for API compatibility |
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.
Let's remove...api compatibility for the init is not really and issue here.
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 remove poll_pause_sec from the class parameter list.
| # Wait before reconnecting | ||
| await asyncio.sleep(2) | ||
|
|
||
| async def _process_single_run(self): |
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.
I'm not sure I understand why we would want to process a single run in isolation. For testing?
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.
I removed single_run from the class parameter list and also removed the _process_single_run method.
| msg_type = message.get('type') | ||
|
|
||
| if msg_type == 'run_start': | ||
| await self._handle_run_start(message) |
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.
just so you know, these are likely to be pretty different when we get the real interface. "new_frame" will be an "event" that we have to do a fair amount of processing with
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 for the reminder. I will update them once we have the real interface. Let’s take a closer look tomorrow.
|
|
||
| # Create operator and publisher | ||
| operator = TiledRawFrameOperator() | ||
| publisher = ZMQFramePublisher.from_settings(app_settings.zmq_frame_publisher) |
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 raises an interesting architectural quesion.
Right now, we have Tiled WS sending events to one listener which has one publisher. Then, we'll be having multiple ZMQ subscribers listening to that publisher.
Our system could be made easier by eliminating the zmq altogether and having the LSEOperator and VizOperator both establishing their own web socket connects to tiled.
For now I guess it makes sense to keep it as is, since we'll be using hardware in Berkeley, and it means less data will come over the wire.
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.
Sure, let’s leave it as is for now and address it in a future PR.
| else: | ||
| logger.debug(f"Ignoring message type: {msg_type}") | ||
|
|
||
| async def _handle_run_start(self, message): |
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.
As a general practice, I would love to see type hints for all methods. I know that my code that you read doesn't have return types (it should) but, especially for framework code like this, I would really like to type hint everything.
|
@dylanmcreynolds Hi Dylan, I still have some questions about the newly added Remaining Issues with new tiled_websocket.pya. Uncertainty About When to Call b. Placement of c. Incomplete Question 1: Since Question 2: It's unclear whether Question3: Does |
publish_stopThe tiled websocket service does not yet publish a stop event, so we have nothing to go on. This is fine for now, our application can get by with users refreshing the screen between scans. publish_eventGood catch. Let's create a new PR? incompleteYeah, we're going to have to make somethings up. It was hard to do any of this without the tiled websocket simulator. Question 1Makes sense. Question 2Good catch. I don't really know. I think we're go with the exact index that tiled gives us so that we can easily match. Question 3This is still a work in progress on the tiled websocket server, so we don't really know yet. We should find out Monday or Tuesday. |
add new arroyo listener according to https://app.asana.com/1/4262699445529/project/1210776618867217/task/1210805079681380?focus=true
add a results publisher:
https://app.asana.com/1/4262699445529/project/1210776618867217/task/1210827537370723?focus=true
add websocket simulator for the new websocket listener
https://app.asana.com/1/4262699445529/project/1210776618867217/task/1210827537370730?focus=true