-
Notifications
You must be signed in to change notification settings - Fork 69
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
feat: expose decompression_executor and interpretation_executor in uproot dask #1120
Conversation
This PR doesn't add tests; however, it was tested locally to make sure the executors are exposed correctly. I used the tests in file All seems to be working well :) |
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 would expect this:
class TestExecutor(uproot.source.futures.TrivialExecutor):
def __init__(self):
self.submit_count = 0
def submit(self, task, /, *args, **kwargs):
self.submit_count += 1
super().submit(task, *args, **kwargs)
testexecutor = TestExecutor()
a = uproot.dask({
skhep_testdata.data_path("uproot-Zmumu-zlib.root"): "events"
}, compression_executor=testexecutor)
a["pt1"].compute()
to result in a testexecutor.submit_count
that is not 0
. (Same for interpretation_executor
.)
Am I testing it incorrectly? What did you test to see that it worked?
In fact, if I replace the counter with raise Exception
, a["pt1"].compute()
returns a result without anything raising an exception. Uproot is not calling submit
on the provided executor.
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.
Okay, great! Looking at the differences since last time, it was just a matter of passing the arguments further down (into those "UprootOpenAndRead" classes), not that the executors aren't working even on eager reads.
This is good to merge! Squash-and-merge whenever you're ready!
The Protocol was not updated alongside the implementations |
#1163 fixes that. Thanks! |
No description provided.