fix(fs): add dirs caching, make it a bit more robust #322
Conversation
| cache["ids"][item_id] = path | ||
| cache["dirs"][path].append(item_id) |
There was a problem hiding this comment.
C: This is better to do in this order since we always refer first to dirs to decide id item is cached or not. Still looks fragile, and we still ideally should be using proper locking. I'll looking into this as a follow up.
There was a problem hiding this comment.
IIRC, this is not true in find where we use ids first.
There was a problem hiding this comment.
Ahh, I see that you also changed find below.
| res.append(item["id"]) | ||
|
|
||
| if item["mimeType"] == FOLDER_MIME_TYPE: | ||
| self._cache_path_id( |
There was a problem hiding this comment.
C: This is the main change here. It was missing and DVC kept hitting _get_remote_item_ids with the same queries like files/md5, etc.
| dir_ids = self._ids_cache["dirs"][base] | ||
| else: | ||
| dir_ids = self._path_to_item_ids(base) | ||
| dir_ids = self._path_to_item_ids(base) |
There was a problem hiding this comment.
C: since now self._path_to_item_ids(base) also does caching it's not clear why we should have a separate check above. I need to review this one more time though. The same applies in the find below, and removal of the cache calls since we now call cache inside path_to_item_ids.
shcheklein
force-pushed
the
fix-fsspec-cache-management
branch
from
a7fa3e3 to
2040665
Compare
Caching mechanics was broken in certain scenarios and affected DVC tremendously. Before we were assuming pre-cached paths for
00,01....ffin DVC. It got broken during DVC 3.0 release, and then got removed completely because of this. Now it's not the case and DVC was kept asking forfiles,files\md5, etc - all those IDs again and again and again ondvc pull. Pretty much every object could be 3x slower to download.Among other things, I checked that
dvc pullworks now even with duplicated root dirs in DVC remote.Still need to self review this, and next see if we can add tests for all of this. It keeps biting us. Caching is non trivial and we cant be losing the knowledge as we go, every time coming back to fix it after users complain.