-
Notifications
You must be signed in to change notification settings - Fork 5
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
HICAT-994 Device servers using Python shared memory managers #243
HICAT-994 Device servers using Python shared memory managers #243
Conversation
38095ad
to
859fd71
Compare
@@ -15,3 +18,24 @@ def apply_shape_to_both(self, dm1_shape, dm2_shape): | |||
@abstractmethod | |||
def apply_shape(self, dm_shape, dm_num): | |||
"""Forms a command for a single DM, with zeros padded for the DM not in use.""" | |||
|
|||
# # A "static" (non-autoproxy) proxy example. NOTE: Using this makes no difference to performance. |
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.
Kept as an example.
f39ad0f
to
7c762ac
Compare
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.
Aside from the (trivial) merge conflict during the merge of #240, I don't see much wrong with this PR. I approve this PR for merging, and will not do detailed notes since that's totally uninteresting for everyone.
Some general comments.
- A lot of the code in this PR is very advanced, to the point that maintenance will likely (read: certainly) become an issue in the future. However, I feel that this is to some degree unavoidable with the current state of catkit.
- The mutex patterns are still unclear to me. This is the main concern for me for future unexpected bugs (from a user perspective at least).
- I really like the new
Testbed
class, making it do the safety checks rather than an experiment. Great separation of concerns. - Thanks for the slight refactor of the data log into a context manager.
- Documentation is a bit sparse. Most internal comments are not accessible to a non-expert, and I presume even an expert that is reading it for the first time. However,
catkit/testbed/README.md
is a good start for people wanting to learn about the internals of catkit multiprocessing. Especially the example in "Hardware access full stack flow" is a good first step. - The signal/events for experiment lifetime and safety are very well structured. See also the arguments of
Experiment.__init__()
for a further description.
STOP_EVENT = "catkit_stop_event" | ||
FINISH_EVENT = "catkit_soft_stop_event" | ||
SAFETY_EVENT = "catkit_safety_event" | ||
SAFETY_BARRIER = "catkit_safety_barrier" |
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.
No enum? No need to change this; just curious.
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.
🤣 Touché
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'll punt this to another PR.
Signed-off-by: James Noss <[email protected]>
Signed-off-by: James Noss <[email protected]>
Signed-off-by: James Noss <[email protected]>
Signed-off-by: James Noss <[email protected]>
Signed-off-by: James Noss <[email protected]>
Signed-off-by: James Noss <[email protected]>
Signed-off-by: James Noss <[email protected]>
Signed-off-by: James Noss <[email protected]>
Signed-off-by: James Noss <[email protected]>
Signed-off-by: James Noss <[email protected]>
Signed-off-by: James Noss <[email protected]>
* Allow for daemonic experiments. * Return iterator proxy for Camera.stream_exposures. * Add base Instrument proxy that doesn't mutex all client sides calls. * Remove possible deadlock that could result in devices not being closed. * Add NamedEvent * Add FINISH_EVENT to aid user with synchronizing when to finish/stop concurrent experiments. Signed-off-by: James Noss <[email protected]>
Signed-off-by: James Noss <[email protected]>
Signed-off-by: James Noss <[email protected]>
…e we used to Signed-off-by: James Noss <[email protected]>
Signed-off-by: James Noss <[email protected]>
47dc705
to
e38a6da
Compare
Signed-off-by: James Noss <[email protected]>
Signed-off-by: James Noss <[email protected]>
a26dc27
to
b1532c2
Compare
@ehpor Just for the sake of it, would you mind re-approving this PR, please? Note that it's just the upstream hicat CI that has failed, as expected. |
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.
Reapproved.
Upstream PR for https://github.com/spacetelescope/hicat-package/pull/607.
See catkit/testbed/README.md
Changes to existing key features:
Experiment
intocatkit.testbed.experiment.Testbed
. This is mainly just the safety tests, however, the hicat derivation will also include ownership of all of the remote servers, i.e., the exception-handler, all device servers, the testbed state server, and the sim server.Experiment.start()
(that on the parent process) doesn't run the safety checks, these are now run on a child process byExperiment.Testbed
.Experiment.start()
doesn't block the start of another experiment.Experiment.start()
doesn't wait and block until the experiment completes, it returns just asProcess.start()
does.Experiment.join()
has been added and mimicsProcess.join()
.Experiment.run_experiment()
(that run on the child process) monitors synchronization events from a dedicated monitor thread. This monitor thread will interrupt the main thread (that runningExperiment.experiment()
) upon certain events.finally
statement.