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

Async with real detector #30

Merged
merged 7 commits into from Jul 27, 2021
Merged

Async with real detector #30

merged 7 commits into from Jul 27, 2021

Conversation

uellue
Copy link
Member

@uellue uellue commented Jul 23, 2021

Edit: Now closes #29 thanks to fix by @sk1p!

This PR is just a failing test case since I couldn't get it to work properly. It is based on #29

The reason that it fails is that we have to keep the connection until the computation is complete, i.e. until the result is awaited or the async generator consumed. The sync case is fixed in #29.

@sk1p could you have a look? I am not sure how live processing in an async context
should be implemented, since we have to read the data in the proper
sequence from one socket.

Use case for this API would probably be integration in an async GUI application? Or should we not support async at all and throw a NotImplementedError if sync is False?

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

I couldn't get this to work, sadly. Will become a bug report.
This fails currently, and I couldn't get it to work with reasonable effort.

The root cause is that the context manager to make the connection
doesn't work properly. I am not sure how live processing in an async context
should be implemented, since we have to read the data in the proper
sequence from one socket.
@uellue uellue added the bug Something isn't working label Jul 23, 2021
@uellue uellue added this to the 0.2 milestone Jul 23, 2021
@uellue
Copy link
Member Author

uellue commented Jul 23, 2021

/azp run libertem.libertem-live-data

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@codecov
Copy link

codecov bot commented Jul 23, 2021

Codecov Report

Merging #30 (4c9c1d4) into master (c203202) will decrease coverage by 0.03%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #30      +/-   ##
==========================================
- Coverage   74.00%   73.97%   -0.04%     
==========================================
  Files          14       15       +1     
  Lines        1208     1241      +33     
  Branches      142      145       +3     
==========================================
+ Hits          894      918      +24     
- Misses        276      284       +8     
- Partials       38       39       +1     
Impacted Files Coverage Δ
src/libertem_live/detectors/merlin/data.py 71.30% <72.72%> (-0.35%) ⬇️
src/libertem_live/detectors/common.py 76.00% <76.00%> (ø)
src/libertem_live/api.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c203202...4c9c1d4. Read the comment docs.

@uellue uellue requested a review from sk1p July 23, 2021 09:28
@sk1p
Copy link
Member

sk1p commented Jul 26, 2021

Use case for this API would probably be integration in an async GUI application? Or should we not support async at all and throw a NotImplementedError if sync is False?

I think it does make sense to support the async case - especially if lots of things need to be orchestrated, lots of network connections need to be handled concurrently etc. it would be useful to write the control script or notebook using async.

I'll have a look at the PR changes next.

Instead of overriding `run_udf` and `run_udf_iter`, override only
`_run_sync`, which is maybe a more convenient place: the `sync=True`, `iterare=True`
variant is used in all cases, just more or less wrapped or reduced.
@sk1p
Copy link
Member

sk1p commented Jul 26, 2021

/azp run libertem.libertem-live-data

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sk1p
Copy link
Member

sk1p commented Jul 26, 2021

I pushed my changes into your branch, as discussed offline. This should now be a general solution, for all cases, and should thus supersede #29.

@sk1p
Copy link
Member

sk1p commented Jul 26, 2021

The py36 tests are failing again - this should be unrelated to the current PR, The py36 tests are failing again - this should be unrelated to the current PR, but I'll have a look at maybe stress testing py36 a bit tomorrow.

@uellue
Copy link
Member Author

uellue commented Jul 26, 2021

Nice, thank you! 👍

@uellue uellue changed the title WIP: Async with real detector (failing test case) Async with real detector (failing test case) Jul 27, 2021
@uellue
Copy link
Member Author

uellue commented Jul 27, 2021

/azp run libertem.libertem-live-data

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@uellue uellue changed the title Async with real detector (failing test case) Async with real detector Jul 27, 2021
@sk1p
Copy link
Member

sk1p commented Jul 27, 2021

/azp run libertem.libertem-live-data

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@uellue
Copy link
Member Author

uellue commented Jul 27, 2021

Regarding the intermittent test failures: @sk1p now has better error handling that will hopefully give better insight on what happens there. We'll merge for now and see what happens!

@uellue uellue merged commit 877d451 into LiberTEM:master Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants