-
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
fix writing via XRootD (called from uproot) #76
base: main
Are you sure you want to change the base?
Conversation
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 ok on first pass. Can you add tests for the new methods?
0d311e4
to
fc93c50
Compare
How about adding a writing via XRootD test in uproot? That way I don't have to re-write those functions in a test here, but just add the minimal reproducer that we saw causing the crash as a test there. Also, I see that in the CI we run the uproot tests anyways. |
49cb5e9
to
06bf183
Compare
57b8e2a
to
971fc86
Compare
b1fa979
to
637a4f3
Compare
for more information, see https://pre-commit.ci
can we maybe run the tests? I tried them locally and the only one failing seems to be |
For the test I was thinking just to make a variant of fsspec-xrootd/tests/test_basicio.py Lines 177 to 183 in d33c203
r+ mode.
|
Even better if it seeks and overwrites a bit as well |
This PR fixes scikit-hep/uproot5#1252.
Touching (and truncating) a file on EOS (implemented here) needs the
File
interface in XRootD (as explained here).After this, other changes were needed, especially to allow for the
r+b
mode used in uproot when opening a file. To do this, some functions such asseek
,write
,read
where re-implemented instead of using the default ones inAbstractBufferedFile
.Note: I believe some of these changes do not follow the logic that was originally implemented, so indications in this sense are much appreciated.