Skip to content

Conversation

@ap--
Copy link
Contributor

@ap-- ap-- commented Oct 17, 2025

This PR closes #1930.

It provides a solution to remove the "fo" keyword argument from being considered for the filesystem instance cache. Instead of specializing just on "fo" it adds support for configuring which storage options to skip via a class attribute, similar to how _extra_tokenize_attributes let's users add class attributes to the tokenization.

Note: implementing this revealed some cross-contamination of the tests in test_core.py via the simplecache instance cache. I fixed this by adding a fixture that clears the caches for every test.

@ap-- ap-- marked this pull request as draft October 17, 2025 22:50
@ap--
Copy link
Contributor Author

ap-- commented Oct 17, 2025

fails for "zip://afile::simplecache::ftp://user:pass@localhost:2121/archive.zip", need to revisit implementation...

@ap--
Copy link
Contributor Author

ap-- commented Oct 17, 2025

Looks like this is cross polution via the instance cache between tests using simplecache:

Running test_core.py::test_url_to_fs and test_core.py::test_chained_url in this order fails:

❯ python -m pytest -x fsspec/tests/test_core.py::test_url_to_fs fsspec/tests/test_core.py::test_chained_url -v 
==================================================== test session starts =====================================================
platform darwin -- Python 3.12.2, pytest-8.4.2, pluggy-1.6.0 -- /Users/andreaspoehlmann/Development/filesystem_spec/.venv/bin/python
cachedir: .pytest_cache
benchmark: 5.1.0 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
rootdir: /Users/andreaspoehlmann/Development/filesystem_spec
configfile: pyproject.toml
plugins: mock-3.15.1, recording-0.13.4, asyncio-1.2.0, benchmark-5.1.0, rerunfailures-16.1, cov-7.0.0
asyncio: mode=Mode.STRICT, debug=False, asyncio_default_fixture_loop_scope=None, asyncio_default_test_loop_scope=function
collected 2 items                                                                                                            

fsspec/tests/test_core.py::test_url_to_fs PASSED                                                                       [ 50%]
fsspec/tests/test_core.py::test_chained_url FAILED                                                                     [100%]

========================================================== FAILURES ==========================================================

And passes in the opposite order:

❯ python -m pytest -x fsspec/tests/test_core.py::test_chained_url fsspec/tests/test_core.py::test_url_to_fs -v 
==================================================== test session starts =====================================================
platform darwin -- Python 3.12.2, pytest-8.4.2, pluggy-1.6.0 -- /Users/andreaspoehlmann/Development/filesystem_spec/.venv/bin/python
cachedir: .pytest_cache
benchmark: 5.1.0 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
rootdir: /Users/andreaspoehlmann/Development/filesystem_spec
configfile: pyproject.toml
plugins: mock-3.15.1, recording-0.13.4, asyncio-1.2.0, benchmark-5.1.0, rerunfailures-16.1, cov-7.0.0
asyncio: mode=Mode.STRICT, debug=False, asyncio_default_fixture_loop_scope=None, asyncio_default_test_loop_scope=function
collected 2 items                                                                                                            

fsspec/tests/test_core.py::test_chained_url PASSED                                                                     [ 50%]
fsspec/tests/test_core.py::test_url_to_fs PASSED                                                                       [100%]

===================================================== 2 passed in 2.09s ======================================================

@ap-- ap-- marked this pull request as ready for review October 18, 2025 09:29
@martindurant
Copy link
Member

I don't know why that's happening :|

For the sake of tests, it would make sense to both clear the cache between tests, and also to have some tests that specifically examine the state of the cache after particular operations.

@ap--
Copy link
Contributor Author

ap-- commented Oct 21, 2025

I worked out that it's a stale socket that survives due to the instance cache not being cleared. Below is a reproduction:

def test_url_to_fs_chained_url():
    # Reproduce the test cross-contamination issue:
    #  When test_url_to_fs is executed before test_chained_url the latter fails.
    #  Switching the test execution order makes tests succeed
    #  Cleaning all instance caches between tests makes tests succeed
    import subprocess
    import sys
    import tempfile
    import time
    from fsspec.implementations.ftp import FTPFileSystem
    from fsspec.implementations.cached import CachingFileSystem
    from fsspec.implementations.cached import SimpleCacheFileSystem

    # clear instance caches of classes and subclasses
    classes = [FTPFileSystem, CachingFileSystem]
    while classes:
        cls = classes.pop()
        classes.extend(cls.__subclasses__())
        cls.clear_instance_cache()

    # start the ftpserver process
    host, port, username, password = "localhost", 2121, "user", "pass"
    P = subprocess.Popen(
        [
            sys.executable,
            "-m", "pyftpdlib",
            "-d", str(tempfile.mkdtemp()),
            "-u", username,
            "-P", password,
            "-w"
        ]
    )
    time.sleep(1)

    # The following has to happen in test_url_to_fs
    try:
        fs, url = fsspec.core.url_to_fs(
            f"simplecache::ftp://{username}:{password}@{host}:{port}/afile"
        )
        # The bug will only trigger if we use a method that 
        # internally calls a method on FTPFileSystem().ftp.<some-method>
        fs.exists(url)
    finally:
        # We now kill the ftp server (the ftp_writable fixture does this)
        P.terminate()
        P.wait()

    # if we now run any method on this filesystem, we reproduce the error
    # fs.open("???") <-- would raise the EOFError
    # (because ftplib.FTP keeps a reference to a socket)

    # In the test cross contamination case, the filesystem
    # survives via the instance cache and gets reused in the following test
    assert len(SimpleCacheFileSystem._cache) == 1
    cached_fs = next(iter(SimpleCacheFileSystem._cache.values()))
    assert cached_fs.storage_options == {
        "target_options": {
            "host": host,
            "password": password,
            "port": port,
            "username": username,
        },
        "target_protocol": "ftp",
    }

    # when test_chained_url 
    fs, _ = fsspec.core.url_to_fs(f"simplecache::ftp://{username}:{password}@{host}:{port}/xxx")
    assert fs is cached_fs  # we retrieved the broken fs from the previous section
    fs.open("???")  # raises EOFError

@ap--
Copy link
Contributor Author

ap-- commented Oct 21, 2025

and also to have some tests that specifically examine the state of the cache after particular operations.

I'll add more tests for this.

@ap-- ap-- force-pushed the simplecache-tokenize-without-fo branch from 454bf92 to 48d5fa8 Compare October 25, 2025 12:00
@ap--
Copy link
Contributor Author

ap-- commented Oct 25, 2025

Note to self:

https://github.com/fsspec/filesystem_spec/actions/runs/18803845822/job/53654986219#step:4:2311

The reference filesystem key errors must have been masked somehow by the instance caches not being cleared between tests... The errors occur because the fss dict uses the "wrong" protocol for http filesystems... (http, instead of https) I have to see if this is different for some reason without instance cache clearing...

It's also strange because I can't reproduce this locally on my mac...

@ap-- ap-- marked this pull request as draft October 25, 2025 15:53
@ap--
Copy link
Contributor Author

ap-- commented Oct 29, 2025

This is ready for review.

  • adds support for configuring which storage options to skip via a class attribute, similar to how _extra_tokenize_attributes let's users add class attributes to the tokenization
  • adds a test to ensure simplecache tokenization is independent of fo
  • adds a helper and a test to inspect instance cache sizes

@ap-- ap-- marked this pull request as ready for review October 29, 2025 09:10
@martindurant
Copy link
Member

This looks perfect, thank you - I have no changes at all.

@martindurant martindurant merged commit a6b9954 into fsspec:master Oct 30, 2025
10 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.

SimpleCacheFileSystem._cache grows with every unique chained urlpath fsspec.open call

2 participants