-
Notifications
You must be signed in to change notification settings - Fork 13
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
Make get_fastccd_images() work again #67
Conversation
Codecov Report
@@ Coverage Diff @@
## master #67 +/- ##
=======================================
Coverage 28.72% 28.72%
=======================================
Files 10 10
Lines 282 282
=======================================
Hits 81 81
Misses 201 201
Continue to review full report at Codecov.
|
According to the discussion for this PR in Slack (starting around here: https://lightsource2.slack.com/archives/GBJDHCSTX/p1571145405158100; it's a bit scattered...), a two-step strategy was determined: First, we merge this PR to revert the previous mistaken change so that the tool works. Second, we overhaul csxtools to use new DataBroker APIs. Most likely the usage of the |
Just FYI, this issue was brought up to @wen-hu yesterday. |
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.
Note that this partially reverts #61, which seems to have introduced a breaking change that was unnoticed until now because the version of the csxtools deployed for users on JupyterHub is old.
I endorse this approach until we have the capacity to more comprehensively update csxtools.
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.
Looks fine for now, as a short-term workaround, as @danielballan mentioned.
The PR description still says that it should not be merged, which is misleading. @leofang, do you mind to correct it?
Sadly, I didn't catch the issue in my review of #61 last year. #61 (comment) mentioned the issue, but it didn't bring enough attention. Hopefully we can resolve it soon. |
Sorry @danielballan @mrakitin, I was being sloppy and didn't test the case of multiple headers. The bug is now fixed in 23d6e6a so that this works as expected: from databroker import Broker
from csxtools import get_fastccd_images
db = Broker.named('csx')
stack = get_fastccd_images((db[122630], db[122631]), (db[122627], db[122628], db[122629])) |
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 fix, @leofang! Looks good to me.
With the current version it is not possible for
get_fastccd_images()
to work as expected:This happens if running on CSX servers with packages installed from the defaults-2019C3.0 channel, but not on JupyerHub srv1 & srv2 (current).
This PR is needed to get
get_fastccd_images()
working. Also, the ptychography GUI currently relies on this PR (see NSLS-II/ptycho_gui#65).As discussed below, all csxtools codes relying on slicerator's
@pipeline
decorator need to be re-worked to be compatible with Databroker's new API.Closes #64.