DECTRIS support; use PipelinedExecutor for live processing #51

sk1p · 2022-05-09T15:24:01Z

Support for DECTRIS detectors supporting the SIMPLON API, including support for the new PipelinedExecutor.

This also updates the Merlin support to use the new PipelinedExecutor.

TODO

Update tests
Clean up merlin support (remove old thread pool etc.)
Add official example notebook for dectris
dectris sim doesn't seem to run on Py36, fix or skip
request and add new DEigerClient copyright notice
Fix Py310 merlin tests - sim doesn't start?
Add user guide and reference documentation

Contributor Checklist:

I have added or updated my entry in the creators.json file
I have added a changelog entry for my contribution
I have added/updated documentation for all user-facing changes
I have added/updated test cases

Reviewer Checklist:

/azp run libertem.libertem-live-data passed

sk1p · 2022-05-11T09:13:24Z

/azp run libertem.libertem-live-data

azure-pipelines · 2022-05-11T09:13:36Z

Azure Pipelines successfully started running 1 pipeline(s).

codecov · 2022-05-11T09:15:01Z

Codecov Report

Merging #51 (06dfcac) into master (34dc81b) will decrease coverage by 8.37%.
The diff coverage is 65.73%.

@@            Coverage Diff             @@
##           master      #51      +/-   ##
==========================================
- Coverage   81.93%   73.56%   -8.38%     
==========================================
  Files          17       23       +6     
  Lines        1467     2474    +1007     
  Branches      218      388     +170     
==========================================
+ Hits         1202     1820     +618     
- Misses        216      551     +335     
- Partials       49      103      +54

Impacted Files	Coverage Δ
src/libertem_live/udf/monitor.py	`100.00% <ø> (ø)`
...rc/libertem_live/detectors/dectris/DEigerClient.py	`45.59% <45.59%> (ø)`
src/libertem_live/detectors/dectris/sim.py	`49.00% <49.00%> (ø)`
src/libertem_live/detectors/merlin/sim.py	`75.99% <60.00%> (-1.24%)`	⬇️
src/libertem_live/detectors/dectris/acquisition.py	`80.93% <80.93%> (ø)`
src/libertem_live/detectors/merlin/data.py	`81.38% <81.15%> (+1.08%)`	⬆️
src/libertem_live/udf/record.py	`91.30% <91.30%> (ø)`
src/libertem_live/detectors/merlin/acquisition.py	`94.01% <92.47%> (-1.14%)`	⬇️
src/libertem_live/detectors/dectris/mock.py	`96.22% <96.22%> (ø)`
src/libertem_live/api.py	`100.00% <100.00%> (ø)`
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

sk1p · 2022-05-11T09:26:09Z

/azp run libertem.libertem-live-data

azure-pipelines · 2022-05-11T09:26:19Z

Azure Pipelines successfully started running 1 pipeline(s).

sk1p · 2022-06-29T13:37:25Z

@matbryan52 pushed an example notebook into the prototypes folder in 407032d

matbryan52 · 2022-07-05T13:23:14Z

@matbryan52 pushed an example notebook into the prototypes folder in 407032d

Thanks! Finally had a moment to look at this and can confirm I got it running on a MIB dataset acquired here. Just had to increase the maximum open files limit as it's a one-file-per-frame type of dataset. I admit it also took me a while to figure out I needed to use the --wait-trigger argument on the simulator too (otherwise the reciever got a frame for the header bytes).

sk1p · 2022-07-06T10:30:14Z

Thanks! Finally had a moment to look at this and can confirm I got it running on a MIB dataset acquired here. Just had to increase the maximum open files limit as it's a one-file-per-frame type of dataset. I admit it also took me a while to figure out I needed to use the --wait-trigger argument on the simulator too (otherwise the reciever got a frame for the header bytes).

Thank you for testing - yeah, in the other example notebooks, we had docs on this directly in the notebook, which I forgot to add to this one. Sorry about that, hope it didn't waste too much time. There is a question if we should change the defaults to something sensible - @uellue what do you think? I'm mostly starting the simulator with --cached memfd --wait-trigger these days...

FYI: the example notebook needs updates again, as I changed the PipelinedExecutor signature a bit for supporting CUDA etc.

matbryan52 · 2022-07-06T10:41:45Z

FYI: the example notebook needs updates again, as I changed the PipelinedExecutor signature a bit for supporting CUDA etc.

Yep, I just updated (I had to force update the branch bizarrely ?) and am letting the executor decide its own spec. Still working. Am going to try to look more into the code this afternoon, around the meetings.

Is there anything in particular that I should focus on at this point?

I haven't tried --cached memfd yet, will do this too.

uellue · 2022-07-06T10:48:40Z

There is a question if we should change the defaults to something sensible - @uellue what do you think? I'm mostly starting the simulator with --cached memfd --wait-trigger these days...

Yes, sounds good to change the trigger default. Since we have the "experimental" disclaimer in the docs, we can probably just change it with a changelog notice. I usually don't cache and didn't notice much of a difference. Also, memfd is only supported on Linux. Maybe we can leave the default cache behavior as "no cache", or did you notice any misbehavior?

Regarding trigger, what about still accepting --wait-trigger, changing the default, and using sth. like --wait-trigger=false to get the previous default behavior?

sk1p · 2022-07-06T12:07:54Z

Yes, sounds good to change the trigger default. Since we have the "experimental" disclaimer in the docs, we can probably just change it with a changelog notice. I usually don't cache and didn't notice much of a difference. Also, memfd is only supported on Linux. Maybe we can leave the default cache behavior as "no cache", or did you notice any misbehavior?

I don't really use the non-cached case, so 🤷 I mostly built the memfd cached variant for benchmarking purposes - when I built it, it gave a lot better performance than the non-cached case, and was quite useful for "sniffing out" the bottlenecks in receiving, decoding, ... and good preparation work for supporting faster detectors in the future.

Directly comparing, without --cached=memfd I get about 1.1GiB/s, with cache I get 4.5GiB/s.

Regarding trigger, what about still accepting --wait-trigger, changing the default, and using sth. like --wait-trigger=false to get the previous default behavior?

Sounds good - means we don't have to update our notebooks, probably...

Let's do this in a separate PR.

sk1p · 2022-07-06T12:22:01Z

Yep, I just updated (I had to force update the branch bizarrely ?)

Sorry about that, that was me rebasing to current upstream/master to resolve conflicts and make the PR merge-able again.

and am letting the executor decide its own spec. Still working. Am going to try to look more into the code this afternoon, around the meetings.

👍

Is there anything in particular that I should focus on at this point?

For testing, mostly running different UDFs would be valuable - to sniff out any issues around serialization or scattering for example. Anything that is a bit more involved than the UDFs included with LiberTEM itself would be useful. Also, trying "edge cases" around error handling or startup/shutdown sequences.

One current limitation in the PipelinedExecutor is that there no support for concurrently running multiple tasks, or running arbitrary functions while running a task, like the GUI would do.

As for code review, I think the most important parts are the new interfaces in libertem.common.executor, and how they are used by both the PipelinedExecutor and the merlin/dectris acquisition implementations. Feel free to ask any questions, ask for clarifications, and suggest alternatives - I think a lot of the code is also still missing docstrings, so if anything is unclear, please bug me to document it!

matbryan52 · 2022-07-06T12:37:25Z

Understood, just for now I've started writing / implementing some test cases without looking too deeply at the code.

In fact I already found one issue where a UDF raising a bare Exception seems to hang the test suite (while normal derived exceptions seem to complete correctly (and raise RuntimeError)).

uellue · 2022-07-22T16:14:03Z

/azp run libertem.libertem-live-data

azure-pipelines · 2022-07-22T16:14:12Z

Azure Pipelines successfully started running 1 pipeline(s).

uellue · 2022-07-22T16:19:43Z

It seems we can tackle running the tests in CI as soon as LiberTEM 0.10 is ~~developed~~ released?

sk1p · 2022-07-28T17:56:50Z

/azp run libertem.libertem-live-data

azure-pipelines · 2022-07-28T17:56:59Z

Azure Pipelines successfully started running 1 pipeline(s).

A very simple file format: int64 length field followed by bytes of that length per message.

azure-pipelines · 2022-08-17T12:20:27Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2022-08-17T12:33:10Z

Azure Pipelines successfully started running 1 pipeline(s).

* Switch to RTD theme * Run the dectris sim in docs-check to check the example code * Separate sections for API reference and user documentation * Add verbosity switch in dectris sim to make sphinx doctest happy (it's not yet possible to ignore output of testsetup / testcleanup directives) * dectris sim: catch StopException

for more information, see https://pre-commit.ci

azure-pipelines · 2022-08-18T16:14:23Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2022-08-18T16:59:35Z

Azure Pipelines successfully started running 1 pipeline(s).

for more information, see https://pre-commit.ci

azure-pipelines · 2022-08-18T17:01:26Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2022-08-22T16:48:15Z

Azure Pipelines successfully started running 1 pipeline(s).

sk1p · 2022-08-22T17:09:28Z

request and add new DEigerClient copyright notice

From my perspective, this is the major item still blocking the PR - we could also handle this as a separate issue, but at least before the release the copyright header should be fixed.

Notes on the code coverage:

DEigerClient is obviously not fully tested/covered here
missing tests for correction data usage (not sure if this is included in our data dump?)
little coverage in the DECTRIS sim - mostly: the Python sender, and alternative triggering mechanisms
same in the DectrisAcquisition - tests missing for exts/ints triggering, corrections, some boundary cases
new MerlinCommHandler: missing some edge cases

Further efficiency improvements are definitely possible, but IMHO can and should be done once this is merged (also important to unblock #56)

uellue

Looks good to me! 🏆

@sk1p

Thx @sk1p!

uellue · 2022-08-24T11:41:23Z

/azp run libertem.libertem-live-data

azure-pipelines · 2022-08-24T11:41:34Z

Azure Pipelines successfully started running 1 pipeline(s).

uellue · 2022-08-24T12:16:39Z

Merging!

sk1p added the enhancement New feature or request label May 9, 2022

sk1p force-pushed the dectris-wip branch 2 times, most recently from d3256d6 to 6b4e2e8 Compare May 10, 2022 12:40

uellue mentioned this pull request May 12, 2022

Base shape check in Negotiator.validate LiberTEM/LiberTEM#1254

Open

sk1p force-pushed the dectris-wip branch from e91e601 to e7de1b5 Compare May 18, 2022 08:53

sk1p force-pushed the dectris-wip branch from e7de1b5 to 960e1db Compare June 9, 2022 16:39

sk1p force-pushed the dectris-wip branch from 960e1db to 99ef791 Compare June 23, 2022 13:11

sk1p mentioned this pull request Jul 6, 2022

simulator: change default to wait for a trigger #54

Open

sk1p added this to the 0.2 milestone Jul 23, 2022

sk1p and others added 3 commits July 28, 2022 20:02

WIP: dectris support

50a6dec

Real Dectris interface code

ddd5500

Record and replay ZMQ messages

c24ea05

A very simple file format: int64 length field followed by bytes of that length per message.

sim: increase timeout; more lazy importing

f6abec7

This comment was marked as duplicate.

Sign in to view

sk1p and others added 4 commits August 18, 2022 15:40

Add more tests for DectrisAcquisition

5f32333

Let docs-check have access to test data

f2c6a83

[pre-commit.ci] auto fixes from pre-commit.com hooks

549023f

for more information, see https://pre-commit.ci

sk1p added 2 commits August 18, 2022 18:49

Add dectris example notebook

0e86c4b

Add changelog entries

eb274b9

This comment was marked as outdated.

Sign in to view

[pre-commit.ci] auto fixes from pre-commit.com hooks

312f456

for more information, see https://pre-commit.ci

uellue mentioned this pull request Aug 19, 2022

WIP: Basic X-Spectrum support #56

Open

5 tasks

Also run the DECTRIS simulator in the background

2ff5fc0

sk1p changed the title ~~WIP: DECTRIS support; use PipelinedExecutor for live processing~~ DECTRIS support; use PipelinedExecutor for live processing Aug 22, 2022

sk1p requested a review from uellue August 22, 2022 17:11

uellue added 2 commits August 24, 2022 13:30

Update contributors and creators

bb6da73

Documentation and example review

4389c70

uellue approved these changes Aug 24, 2022

View reviewed changes

Fix copypasta

06dfcac

Thx @sk1p!

uellue merged commit abd612f into LiberTEM:master Aug 24, 2022

sk1p deleted the dectris-wip branch August 24, 2022 17:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DECTRIS support; use PipelinedExecutor for live processing #51

DECTRIS support; use PipelinedExecutor for live processing #51

sk1p commented May 9, 2022 •

edited

Loading

sk1p commented May 11, 2022

azure-pipelines bot commented May 11, 2022

codecov bot commented May 11, 2022 •

edited

Loading

sk1p commented May 11, 2022

azure-pipelines bot commented May 11, 2022

sk1p commented Jun 29, 2022

matbryan52 commented Jul 5, 2022

sk1p commented Jul 6, 2022

matbryan52 commented Jul 6, 2022 •

edited

Loading

uellue commented Jul 6, 2022

sk1p commented Jul 6, 2022

sk1p commented Jul 6, 2022

matbryan52 commented Jul 6, 2022

uellue commented Jul 22, 2022

azure-pipelines bot commented Jul 22, 2022

uellue commented Jul 22, 2022 •

edited

Loading

sk1p commented Jul 28, 2022

azure-pipelines bot commented Jul 28, 2022

azure-pipelines bot commented Aug 17, 2022

This comment was marked as duplicate.

azure-pipelines bot commented Aug 17, 2022

azure-pipelines bot commented Aug 18, 2022

This comment was marked as outdated.

azure-pipelines bot commented Aug 18, 2022

azure-pipelines bot commented Aug 18, 2022

azure-pipelines bot commented Aug 22, 2022

sk1p commented Aug 22, 2022

uellue left a comment

uellue commented Aug 24, 2022

azure-pipelines bot commented Aug 24, 2022

uellue commented Aug 24, 2022

DECTRIS support; use PipelinedExecutor for live processing #51

DECTRIS support; use PipelinedExecutor for live processing #51

Conversation

sk1p commented May 9, 2022 • edited Loading

TODO

Contributor Checklist:

Reviewer Checklist:

sk1p commented May 11, 2022

azure-pipelines bot commented May 11, 2022

codecov bot commented May 11, 2022 • edited Loading

Codecov Report

sk1p commented May 11, 2022

azure-pipelines bot commented May 11, 2022

sk1p commented Jun 29, 2022

matbryan52 commented Jul 5, 2022

sk1p commented Jul 6, 2022

matbryan52 commented Jul 6, 2022 • edited Loading

uellue commented Jul 6, 2022

sk1p commented Jul 6, 2022

sk1p commented Jul 6, 2022

matbryan52 commented Jul 6, 2022

uellue commented Jul 22, 2022

azure-pipelines bot commented Jul 22, 2022

uellue commented Jul 22, 2022 • edited Loading

sk1p commented Jul 28, 2022

azure-pipelines bot commented Jul 28, 2022

azure-pipelines bot commented Aug 17, 2022

This comment was marked as duplicate.

azure-pipelines bot commented Aug 17, 2022

azure-pipelines bot commented Aug 18, 2022

This comment was marked as outdated.

azure-pipelines bot commented Aug 18, 2022

azure-pipelines bot commented Aug 18, 2022

azure-pipelines bot commented Aug 22, 2022

sk1p commented Aug 22, 2022

uellue left a comment

Choose a reason for hiding this comment

uellue commented Aug 24, 2022

azure-pipelines bot commented Aug 24, 2022

uellue commented Aug 24, 2022

sk1p commented May 9, 2022 •

edited

Loading

codecov bot commented May 11, 2022 •

edited

Loading

matbryan52 commented Jul 6, 2022 •

edited

Loading

uellue commented Jul 22, 2022 •

edited

Loading