Old bug (dvc pull from S3 and md5 starts with `00`) reappears with latest release

[jnd77]

Bug Report

dvc pull does not seem to work for files whose md5 starts with 00, when remote is S3

Description

This bug #4141 has reappeared with release 2.2.0 (it's working fine with 2.1.0).
Might be due to this commit:
#5683

Let me know if you need more information ... :)

[isidentical]

Would you mind sending the full traceback?

[isidentical]

I just tried this as a test case and it seemed to work (363's hash is something that startswith 00, which is confirmed in the last assert);

@pytest.mark.parametrize("remote", [pytest.lazy_fixture("s3")], indirect=True)
def test_pull_00_prefix(tmp_dir, dvc, remote):
    tmp_dir.dvc_gen({"foo": "363"})
    dvc.push()
    clean(["foo"], dvc)
    
    stats = dvc.pull("foo")
    assert stats["fetched"] == 1
    assert stats["added"] == ["foo"]
    
    [cache_dir] = dvc.cloud.get_remote('upstream').fs.ls(remote)
    assert cache_dir.endswith('00')

[pmrowla]

@isidentical I think we'd need to test with pulling more than one file at a time, if we're only trying to push/pull a single file, we do the direct exists() query instead of using walk/ls when we are checking whether objects exist in the remote.

It looks like we might not be handling the dvc specific prefix kwarg for fs.walk_files()/walk() any more, if we're just passing it directly into fsspec's fs.ls now, fsspec won't do anything with it.

see #4141 (comment) for an explanation of the original bug, and #4149 (comment) for discussion on why we added the prefix kwarg

essentially , there's times that we do walk("00/0", prefix=True) where we really want to query for S3 keys that start with 00/0, not keys inside 00/0/ (I'm not sure what API call s3fs will actually run in this case?)

[isidentical]

essentially , there's times that we do walk("00/0", prefix=True) where we really want to query for S3 keys that start with 00/0, not keys inside 00/0/ (I'm not sure what API call s3fs will actually run in this case?)

Probably find() is the right API but I recall that discussing this with @efiop and noticing that it doesn't work on arbitrary prefixes. I'm checking out the s3fs but in the meanwhile I assume this optimization is something that we could control (setting the prefix length to 2 should work, right?) so maybe we could do that as an option.

[pmrowla]

(setting the prefix length to 2 should work, right?)

Yeah, this should work in the meantime, but ideally it'd be best if we can use arbitrary prefix lengths longer than 2 for clouds that support it

[efiop]

Also looks like we've added a test in #4149 which is still present right now, but it was only testing the exists() path 🙁

@isidentical Yes, we actually use just 00/ for all non-s3-like clouds (TRAVERSE_PREFIX_LEN), so we could try to fallback to that for now, if there is no better quick way to do that. But find should also work, considering that ls right now doesn't return iterator, but the full list. Both ways mean that we will lose some performance in estimation speed (since we'll have to go through more files), but that is expected at this point in our migration.

[isidentical]

Also looks like we've added a test in #4149 which is still present right now, but it was only testing the exists() path slightly_frowning_face

It would be great if we could add a more generic test to the pull itself. I've modified the case to include additional 1024 files though I think it wasn't enough (still succeeds);

@pytest.mark.parametrize("remote", [pytest.lazy_fixture("s3")], indirect=True)
def test_pull_00_prefix(tmp_dir, dvc, remote):
    random_files = {f'random_{n}': str(n) for n in range(1024)}
    tmp_dir.dvc_gen({"foo": "363", "random": random_files})

    dvc.push()
    clean(["foo", "random"], dvc)
    
    stats = dvc.pull()
    assert 'foo' in stats['added']

[efiop]

@isidentical The behavior depends on TRAVERSE_PREFIX_LEN (@pmrowla could correct me if I'm wrong), so we'll need to exceed that or, probably better, just patch it to be lower for the tests.

EDIT: i meant TRAVERSE_THRESHOLD_SIZE 🤦

[pmrowla]

If you set

BaseFileSystem.TRAVERSE_THRESHOLD_SIZE = 0
BaseFileSystem.TRAVERSE_WEIGHT_MULTIPLIER = 1

in your test, it should force the prefix based query for all push/pull/status queries w/more than one on file all remote types

[isidentical]

Still can't reproduce :/ (Also tried setting LIST_OBJECT_PAGE_SIZE though that didn't work).

@pytest.mark.parametrize("remote", [pytest.lazy_fixture("s3")], indirect=True)
def test_pull_00_prefix(tmp_dir, dvc, remote, monkeypatch):
    from dvc.fs.s3 import BaseFileSystem

    BaseFileSystem.TRAVERSE_THRESHOLD_SIZE = 0
    BaseFileSystem.TRAVERSE_WEIGHT_MULTIPLIER = 1


    random_files = {f'random_{n}': str(n) for n in range(32)}
    tmp_dir.dvc_gen({"foo": "363", "random": random_files})

    dvc.push()
    clean(["foo", "random"], dvc)
    
    stats = dvc.pull()
    assert 'foo' in stats['added']