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

Merged
merged 11 commits into from
Feb 18, 2025

Conversation

maxgalli
Copy link
Contributor

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?

@maxgalli
Copy link
Contributor 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
Contributor 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

@maxgalli
Copy link
Contributor Author

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.

added

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.

Thanks, looks all good now

@nsmith-
Copy link
Member

nsmith- commented Feb 17, 2025

I don't know why the format CI is failing, it looks unrelated to your PR

@nsmith- nsmith- merged commit c749971 into scikit-hep:main Feb 18, 2025
8 checks passed
@nsmith- nsmith- linked an issue Feb 19, 2025 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants