Cache Manifest files #787

chinmay-bhat · 2024-06-02T06:22:33Z

Closes #595.

chinmay-bhat · 2024-06-03T11:28:59Z

One test is failing CI (test_duckdb_url_import).

FAILED tests/integration/test_writes/test_writes.py::test_duckdb_url_import - duckdb.duckdb.IOException: IO Error: Failed to download extension "iceberg" at URL "http://extensions.duckdb.org/v0.10.3/linux_amd64_gcc4/iceberg.duckdb_extension.gz"
Extension "iceberg" is an existing extension.
 (ERROR Connection)
= 1 failed, 797 passed, 5 skipped, 2735 deselected, 160 warnings in 261.13s (0:04:21) =
make: *** [Makefile:46: test-integration] Error 1
Error: Process completed with exit code 2.

Seems to be a download error and not due to changes in this PR.
Also, the same test passes locally. Can we re-trigger the tests?

HonahX · 2024-06-04T07:00:53Z

Hmm, it's my first time to see this error. I've merged a PR that bumps duckdb to 1.0.0: #793. Hope that can fix the issue here.

BTW, thanks for working on this!

pyiceberg/table/snapshots.py

Fokko

LGTM

chinmay-bhat · 2024-06-10T06:01:06Z

@Fokko @HonahX Thanks for the review!!

MehulBatra · 2024-06-10T07:17:54Z

@chinmay-bhat @Fokko @HonahX Sorry for the last-minute feedback, but wouldn't it be better if we explicitly pass
(maxsize=128) for the lru_cache annotation, When a new call comes in, the decorator’s implementation will evict the least recently used of the existing 128 entries to make a place for the new item. that way it would make it clearer for the end user and increase the code visibility, in the near future we can also take that as a user config via pyiceberg.yaml

@lru_cache(maxsize=128)
def _manifests(io: FileIO, manifest_list: str) -> List[ManifestFile]:
    """Return the manifests from the manifest list."""
    file = io.new_input(manifest_list)
    return list(read_manifest_list(file))

MehulBatra · 2024-06-10T07:21:06Z

pyiceberg/table/snapshots.py

@@ -228,6 +229,13 @@ def __eq__(self, other: Any) -> bool:
        )


+@lru_cache


what are your thoughts on this, wouldn't be clearer if we could explicitly pass the max size for the lru_cache annotation, to store max entries rather than going default.
@lru_cache(maxsize=128)

As the default is 128, I don't think we need to explicitly define maxsize=128.

Also, _manifests() is not a public API, so we would probably not set the cache size through user config via pyiceberg.yaml. Is there a use-case where the user would want to set the cache size?

Agree the default size is what you mentioned, It was just an example, but the idea was to make it more readable & configurable.
Please feel free to take your call.

is 128 big enough?

I think 128 should be enough. We're caching manifest_list here and the number of manifest lists should be <100 in most cases.
We can always make this configurable in the future :)

chinmay-bhat · 2024-06-10T08:10:32Z

I'm not sure why test_duckdb_url_import continues to fail with the same error. I've rebased onto latest main branch, and the test runs locally. I also don't see this issue in other open PyIceberg PRs which have recently cleared all tests.

Is this a duck db problem? Or do I need to open a new PR (from main branch + my changes) to resolve it?

Fokko · 2024-06-10T18:58:55Z

Being able to configure (and also disable) the cachine would be a very nice touch Op ma 10 jun 2024 om 09:52 schreef Chinmay Bhat ***@***.***>

…

***@***.**** commented on this pull request. ------------------------------ In pyiceberg/table/snapshots.py <#787 (comment)> : > @@ -228,6 +229,13 @@ def __eq__(self, other: Any) -> bool: ) ***@***.***_cache As the default is 128 <https://docs.python.org/3/library/functools.html#functools.lru_cache>, I don't think we need to explicitly define maxsize=128. Also, _manifests() is not a public API, so we would *probably* not set the cache size through user config via pyiceberg.yaml. Is there a use-case where the user would want to set the cache size? — Reply to this email directly, view it on GitHub <#787 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIU5KBBXVDMDRY62QQEY2LZGVLMNAVCNFSM6AAAAABIUZG46GVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMBWHEZTSOJSGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Fokko · 2024-06-10T21:09:25Z

@chinmay-bhat Looks like the CI is still a bit sad :(

chinmay-bhat · 2024-06-11T16:41:14Z

Being able to configure (and also disable) the cachine would be a very nice touch

Hi @Fokko, I'm curious to know why we would want to support custom sized manifest caches.
I agree user should be able to disable the cache.

Here's my initial implementation for configuring and disabling cache:

<top of file>
MANIFEST_CACHE_SIZE = "manifest-cache-size"

config = Config()
manifest_cache_size = config.get_int(MANIFEST_CACHE_SIZE) if config.get_int(MANIFEST_CACHE_SIZE) else 128
....
@lru_cache(maxsize=manifest_cache_size)
def _manifests(io: FileIO, manifest_list: str) -> List[ManifestFile]:
    """Return the manifests from the manifest list."""
    file = io.new_input(manifest_list)
    return list(read_manifest_list(file))

class Snapshot:
....
    def manifests(self, io: FileIO, use_cache: bool = True) -> List[ManifestFile]:
        """Return the manifests for the given snapshot."""
        if self.manifest_list:
            if use_cache:
                return _manifests(io, self.manifest_list)
            else:
                _manifests.cache_clear()
                return _manifests.__wrapped__(io, self.manifest_list)
        return []

Is this in the right direction?
For reference: how to disable lru_cache and lru_cache doc

Looks like the CI is still a bit sad :(

@Fokko @HonahX I need help resolving this error. Error seems unrelated to the changes in this PR. I've added more info in this comment.

HonahX · 2024-06-12T08:32:44Z

Is this a duck db problem? Or do I need to open a new PR (from main branch + my changes) to resolve it?

I tried this PR locally and did not observe this issue. My testing platforms are

MacOS 14.5 - Apple Silicone
Ubuntu 24.04 - Intel x86/64

It seems this issue can only be reproduced in github action😱:

with lru_cahce, test failed: Honahx test cache manifest pr HonahX/iceberg-python#4
without lru_cache, test passed: test HonahX/iceberg-python#5

chinmay-bhat · 2024-06-12T16:55:18Z

I created a new PR against my fork, and once the GitHub actions failed, I manually re-tried them.
https://github.com/chinmay-bhat/iceberg-python/pull/1/checks?sha=8c2e79a9c62f98c51eb56ae65369a7bf3f6d49f4

With lru_cache,

Python Integration / integration-test (make test-integration) passes.
Python CI / lint-and-test (make test-coverage) fails.

Also, with lru_cache, test-integration and test-coverage both pass locally.
My environment is MacOS 14.4.1 - Apple Silicon M2

kevinjqliu · 2024-09-07T19:14:36Z

FYI this is caused by transient networking failures when downloading the duckdb iceberg extension. Using the pre-release duckdb library (duckdb@1.0.1.dev5313) resolves this issue because retry was added in duckdb#13122.
See duckdb#13808 for more info. Might need to wait for duckdb v1.1.0 release

kevinjqliu · 2024-09-09T18:28:07Z

#1149 bumps duckdb to v1.1.0 which has the PR to retry extension installation

kevinjqliu · 2024-09-09T20:46:41Z

@chinmay-bhat do you mind rebasing off main to re-run CI?

chinmay-bhat · 2024-09-10T13:39:04Z

rebased off main!

kevinjqliu

LGTM! CI finally passed!
Thanks for the contribution

This reverts commit 1971fcf.

chinmay-bhat marked this pull request as ready for review June 2, 2024 06:23

chinmay-bhat force-pushed the cache_manifest_files branch from aa27a48 to 173ddb9 Compare June 3, 2024 17:10

HonahX reviewed Jun 4, 2024

View reviewed changes

pyiceberg/table/snapshots.py Outdated Show resolved Hide resolved

chinmay-bhat force-pushed the cache_manifest_files branch 2 times, most recently from 81a85ab to 43a2f35 Compare June 9, 2024 07:21

Fokko requested changes Jun 9, 2024

View reviewed changes

pyiceberg/table/snapshots.py Outdated Show resolved Hide resolved

Fokko reviewed Jun 10, 2024

View reviewed changes

pyiceberg/table/snapshots.py Outdated Show resolved Hide resolved

Fokko approved these changes Jun 10, 2024

View reviewed changes

chinmay-bhat force-pushed the cache_manifest_files branch from 384526c to 8c2e79a Compare June 10, 2024 05:59

MehulBatra reviewed Jun 10, 2024

View reviewed changes

kevinjqliu mentioned this pull request Sep 9, 2024

Bump duckdb to version 1.1.0 #1149

Merged

chinmay-bhat added 5 commits September 10, 2024 18:48

cache manifests

5b85d1c

update API

fc98648

small fix

00e2f44

move cache to module level

1df0f03

update signature and check

a54c570

chinmay-bhat force-pushed the cache_manifest_files branch from 8c2e79a to a54c570 Compare September 10, 2024 13:35

kevinjqliu approved these changes Sep 10, 2024

View reviewed changes

kevinjqliu merged commit 1971fcf into apache:main Sep 10, 2024
8 checks passed

chinmay-bhat deleted the cache_manifest_files branch September 11, 2024 08:27

kevinjqliu added a commit that referenced this pull request Sep 12, 2024

Revert "Cache Manifest files (#787)"

cddcf2a

This reverts commit 1971fcf.

This was referenced Sep 12, 2024

Revert "Cache Manifest files" #1167

Closed

[feature] reimplement Snapshot manifest cache #1185

Closed

kevinjqliu mentioned this pull request Sep 20, 2024

Use cachetools's LRUCache to cache manifest list #1187

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache Manifest files #787

Cache Manifest files #787

chinmay-bhat commented Jun 2, 2024 •

edited

Loading

chinmay-bhat commented Jun 3, 2024

HonahX commented Jun 4, 2024 •

edited

Loading

Fokko left a comment

chinmay-bhat commented Jun 10, 2024

MehulBatra commented Jun 10, 2024 •

edited

Loading

MehulBatra Jun 10, 2024

chinmay-bhat Jun 10, 2024

MehulBatra Jun 10, 2024 •

edited

Loading

corleyma Sep 9, 2024

kevinjqliu Sep 10, 2024

chinmay-bhat commented Jun 10, 2024 •

edited

Loading

Fokko commented Jun 10, 2024 via email

Fokko commented Jun 10, 2024

chinmay-bhat commented Jun 11, 2024 •

edited

Loading

HonahX commented Jun 12, 2024 •

edited

Loading

chinmay-bhat commented Jun 12, 2024 •

edited

Loading

kevinjqliu commented Sep 7, 2024

kevinjqliu commented Sep 9, 2024

kevinjqliu commented Sep 9, 2024

chinmay-bhat commented Sep 10, 2024

kevinjqliu left a comment

		@@ -228,6 +229,13 @@ def __eq__(self, other: Any) -> bool:
		)


		@lru_cache

Cache Manifest files #787

Cache Manifest files #787

Conversation

chinmay-bhat commented Jun 2, 2024 • edited Loading

chinmay-bhat commented Jun 3, 2024

HonahX commented Jun 4, 2024 • edited Loading

Fokko left a comment

Choose a reason for hiding this comment

chinmay-bhat commented Jun 10, 2024

MehulBatra commented Jun 10, 2024 • edited Loading

MehulBatra Jun 10, 2024

Choose a reason for hiding this comment

chinmay-bhat Jun 10, 2024

Choose a reason for hiding this comment

MehulBatra Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

corleyma Sep 9, 2024

Choose a reason for hiding this comment

kevinjqliu Sep 10, 2024

Choose a reason for hiding this comment

chinmay-bhat commented Jun 10, 2024 • edited Loading

Fokko commented Jun 10, 2024 via email

Fokko commented Jun 10, 2024

chinmay-bhat commented Jun 11, 2024 • edited Loading

HonahX commented Jun 12, 2024 • edited Loading

chinmay-bhat commented Jun 12, 2024 • edited Loading

kevinjqliu commented Sep 7, 2024

kevinjqliu commented Sep 9, 2024

kevinjqliu commented Sep 9, 2024

chinmay-bhat commented Sep 10, 2024

kevinjqliu left a comment

Choose a reason for hiding this comment

chinmay-bhat commented Jun 2, 2024 •

edited

Loading

HonahX commented Jun 4, 2024 •

edited

Loading

MehulBatra commented Jun 10, 2024 •

edited

Loading

MehulBatra Jun 10, 2024 •

edited

Loading

chinmay-bhat commented Jun 10, 2024 •

edited

Loading

chinmay-bhat commented Jun 11, 2024 •

edited

Loading

HonahX commented Jun 12, 2024 •

edited

Loading

chinmay-bhat commented Jun 12, 2024 •

edited

Loading