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: XRootDHelper.exists supports non posix filesystem (object store) #1348

Merged
merged 1 commit into from Feb 5, 2022

Conversation

chaen
Copy link
Contributor

@chaen chaen commented Jan 24, 2022

Description

Change the way XRootDHelper checks the existence of a URL to accommodate for object store, where listing a directory does not make sense.

This was mentioned in an old issue: #809
Until v6.7.0, exists would throw an exception, as described in the issue.
From v6.8.0, this PR #1017 changed the behavior, and exists would unduly return False

This PR uses stat call instead of listing the directory (adding @chrisburr as origin author, as he probably had a reason to do it this way)
A side effect is that exists also works for directories, while before it would return always False (this can easily be changed if it is an issue, these are the comments in the code).

QC

  • The PR contains a test case for the changes or the changes are already covered by an existing test case.
  • The documentation (docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).

Not sure how you go about testing xroot, but this is the test file I used, against real world storages

import pytest
from snakemake.remote.XRootD import XRootDHelper
from snakemake.exceptions import XRootDFileException

allURLs = (
    (
        "echoSingleSlash",
        "root://xrootd.echo.stfc.ac.uk/lhcb:user/lhcb/user/c/chaen/zozo.xml",
        True,
    ),
    (
        "echoDoubleSlash",
        "root://xrootd.echo.stfc.ac.uk//lhcb:user/lhcb/user/c/chaen/zozo.xml",
        True,
    ),
    (
        "echoDoesntExist",
        "root://xrootd.echo.stfc.ac.uk//lhcb:user/lhcb/user/c/chaen/doesntexist.xml",
        False,
    ),
    (
        "echoDirectory",
        "root://xrootd.echo.stfc.ac.uk//lhcb:user/lhcb/user/c/chaen/",
        False,
    ),
    (
        "eosSingleSlash",
        "root://eoslhcb.cern.ch/eos/lhcb/grid/user/lhcb/user/c/chaen/zozo.xml",
        True,
    ),
    (
        "eosDoubleSlash",
        "root://eoslhcb.cern.ch//eos/lhcb/grid/user/lhcb/user/c/chaen/zozo.xml",
        True,
    ),
    (
        "eosDoesntExist",
        "root://eoslhcb.cern.ch//eos/lhcb/grid/user/lhcb/user/c/chaen/notexisting",
        False,
    ),
    (
        "eosDirectoryWithSlash",
        "root://eoslhcb.cern.ch//eos/lhcb/grid/user/lhcb/user/c/chaen/test/",
        True,
    ),
    (
        "eosDirectoryWithoutSlash",
        "root://eoslhcb.cern.ch//eos/lhcb/grid/user/lhcb/user/c/chaen/test/",
        True,
    ),
    (
        "wrongHost",
        "root://eolhcb.cern.ch//eos/lhcb/grid/user/lhcb/user/c/chaen/zozo.xmlsds",
        XRootDFileException,
    ),
)

h = XRootDHelper()


@pytest.mark.parametrize("name,url,expected", allURLs)
def test_exists(name, url, expected):
    if isinstance(expected, bool):
        assert h.exists(url) == expected
    else:
        with pytest.raises(expected):
            h.exists(url)

Result with v6.7.0

(snakemake) [chaen@pceplbc04 myTest]$ pytest  --tb=line  test_exists.py::test_exists
==================================================================================== test session starts =====================================================================================
platform linux -- Python 3.9.9, pytest-6.2.5, py-1.11.0, pluggy-1.0.0
rootdir: /home/chaen/workspace/snakemake
collected 10 items                                                                                                                                                                           

test_exists.py FF..F..FF.                                                                                                                                                              [100%]

========================================================================================== FAILURES ==========================================================================================
/home/chaen/workspace/snakemake/snakemake/remote/XRootD.py:143: snakemake.exceptions.XRootDFileException: Error listing directory lhcb:user/lhcb/user/c/chaen/ on domain root://xrootd.echo.stfc.ac.uk/
/home/chaen/workspace/snakemake/myTest/test_exists.py:64: AssertionError: assert False == True
/home/chaen/workspace/snakemake/snakemake/remote/XRootD.py:143: snakemake.exceptions.XRootDFileException: Error listing directory eos/lhcb/grid/user/lhcb/user/c/chaen/ on domain root://eoslhcb.cern.ch/
/home/chaen/workspace/snakemake/myTest/test_exists.py:64: AssertionError: assert False == True
/home/chaen/workspace/snakemake/myTest/test_exists.py:64: AssertionError: assert False == True
================================================================================== short test summary info ===================================================================================
FAILED test_exists.py::test_exists[echoSingleSlash-root:/xrootd.echo.stfc.ac.uk/lhcb:user/lhcb/user/c/chaen/zozo.xml-True] - snakemake.exceptions.XRootDFileException: Error listing direct...
FAILED test_exists.py::test_exists[echoDoubleSlash-root:/xrootd.echo.stfc.ac.uk/lhcb:user/lhcb/user/c/chaen/zozo.xml-True] - AssertionError: assert False == True
FAILED test_exists.py::test_exists[eosSingleSlash-root:/eoslhcb.cern.ch/eos/lhcb/grid/user/lhcb/user/c/chaen/zozo.xml-True] - snakemake.exceptions.XRootDFileException: Error listing direc...
FAILED test_exists.py::test_exists[eosDirectoryWithSlash-root:/eoslhcb.cern.ch/eos/lhcb/grid/user/lhcb/user/c/chaen/test/-True] - AssertionError: assert False == True
FAILED test_exists.py::test_exists[eosDirectoryWithoutSlash-root:/eoslhcb.cern.ch/eos/lhcb/grid/user/lhcb/user/c/chaen/test/-True] - AssertionError: assert False == True
================================================================================ 5 failed, 5 passed in 3.48s =================================================================================

Results with v6.8.0

(snakemake) [chaen@pceplbc04 myTest]$ pytest  --tb=line  test_exists.py::test_exists
==================================================================================== test session starts =====================================================================================
platform linux -- Python 3.9.9, pytest-6.2.5, py-1.11.0, pluggy-1.0.0
rootdir: /home/chaen/workspace/snakemake
collected 10 items                                                                                                                                                                           

test_exists.py FF.....FF.                                                                                                                                                              [100%]

========================================================================================== FAILURES ==========================================================================================
/home/chaen/workspace/snakemake/myTest/test_exists.py:64: AssertionError: assert False == True
/home/chaen/workspace/snakemake/myTest/test_exists.py:64: AssertionError: assert False == True
/home/chaen/workspace/snakemake/myTest/test_exists.py:64: AssertionError: assert False == True
/home/chaen/workspace/snakemake/myTest/test_exists.py:64: AssertionError: assert False == True
================================================================================== short test summary info ===================================================================================
FAILED test_exists.py::test_exists[echoSingleSlash-root:/xrootd.echo.stfc.ac.uk/lhcb:user/lhcb/user/c/chaen/zozo.xml-True] - AssertionError: assert False == True
FAILED test_exists.py::test_exists[echoDoubleSlash-root:/xrootd.echo.stfc.ac.uk/lhcb:user/lhcb/user/c/chaen/zozo.xml-True] - AssertionError: assert False == True
FAILED test_exists.py::test_exists[eosDirectoryWithSlash-root:/eoslhcb.cern.ch/eos/lhcb/grid/user/lhcb/user/c/chaen/test/-True] - AssertionError: assert False == True
FAILED test_exists.py::test_exists[eosDirectoryWithoutSlash-root:/eoslhcb.cern.ch/eos/lhcb/grid/user/lhcb/user/c/chaen/test/-True] - AssertionError: assert False == True
================================================================================ 4 failed, 6 passed in 1.95s =================================================================================

With this PR

(snakemake) [chaen@pceplbc04 myTest]$ pytest  --tb=line  test_exists.py::test_exists
==================================================================================== test session starts =====================================================================================
platform linux -- Python 3.9.9, pytest-6.2.5, py-1.11.0, pluggy-1.0.0
rootdir: /home/chaen/workspace/snakemake
collected 10 items                                                                                                                                                                           

test_exists.py ..........                                                                                                                                                              [100%]

===================================================================================== 10 passed in 1.29s =====================================================================================

@chaen chaen requested a review from johanneskoester as a code owner Jan 24, 2022
@sonarcloud
Copy link

sonarcloud bot commented Jan 24, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@johanneskoester
Copy link
Contributor

johanneskoester commented Jan 26, 2022

Thanks a lot! Well, ideally we would use an xrootd docker container to setup an instance directly in the github actions:
https://github.com/opensciencegrid/docker-xrootd-standalone

Would you be interested in doing that? It could be done in this PR. The benefit would be that we protect ourselves against future issues that would be unseen otherwise.

@chaen
Copy link
Contributor Author

chaen commented Jan 28, 2022

Hi @johanneskoester
This docker-xrootd-standalone would surely help, but it will not exhibit this corner case of object store that I am trying to fix here. I'll see if I can find something equivalent

@johanneskoester
Copy link
Contributor

johanneskoester commented Feb 5, 2022

Ok, looking forward to that. In the meantime I will merge this and trust your local testing.

@johanneskoester johanneskoester merged commit 7a3ad2f into snakemake:main Feb 5, 2022
7 checks passed
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.

None yet

2 participants