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

fix writing via XRootD (called from uproot) #76

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

maxgalli
Copy link

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 as seek, write, read where re-implemented instead of using the default ones in AbstractBufferedFile.

Note: I believe some of these changes do not follow the logic that was originally implemented, so indications in this sense are much appreciated.

Copy link
Member

@nsmith- nsmith- left a 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?

src/fsspec_xrootd/xrootd.py Outdated Show resolved Hide resolved
src/fsspec_xrootd/xrootd.py Outdated Show resolved Hide resolved
@maxgalli
Copy link
Author

Looks ok on first pass. Can you add tests for the new methods?

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.

@maxgalli
Copy link
Author

can we maybe run the tests? I tried them locally and the only one failing seems to be tests/test_basicio.py::test_touch_modified. However, I tried with the main branch and it fails as well. Since in the last build in main it seems to pass in the CI, it makes me think there might be something different with my local setup.

@nsmith-
Copy link
Member

nsmith- commented Feb 14, 2025

For the test I was thinking just to make a variant of

def test_write_fsspec(localserver, clear_server):
remoteurl, localpath = localserver
with fsspec.open(remoteurl + "/testfile.txt", "wt") as f:
f.write(TESTDATA1)
f.flush()
with open(localpath + "/testfile.txt") as f:
assert f.read() == TESTDATA1
that tests the newly enabled r+ mode.

@nsmith-
Copy link
Member

nsmith- commented Feb 14, 2025

Even better if it seeks and overwrites a bit as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

uproot cannot write root files over xrootd.
2 participants