Makes prefetcher none cache default - #992
Conversation
There was a problem hiding this comment.
Code Review
This pull request enables the adaptive concurrent prefetcher by default, updating the default concurrency from 1 to 4 and dynamically setting the default cache type to "none" when prefetching is active (or "readahead" when disabled). The documentation and test suites have been updated to reflect these new defaults. Feedback on the changes suggests handling potential string values for the prefetching kwarg to avoid incorrect boolean evaluation, updating ZonalFile to align with the new dynamic cache defaults, and wrapping the environment variable integer parsing in a try-except block to prevent import-time crashes.
… include string value
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enables adaptive concurrent prefetching by default, changing the default concurrency from 1 to 4 and setting the default cache type to "none" when prefetching is active. It also updates the documentation and test suite to reflect these new defaults. Regarding the feedback, a discrepancy was identified in how use_experimental_adaptive_prefetching is resolved in GCSFile.__init__ compared to ZonalFile.__init__. Specifically, passing None explicitly in kwargs would evaluate to False and disable prefetching while keeping cache_type="none", resulting in no caching or prefetching at all. A code suggestion has been provided to resolve this consistently.
| if "use_experimental_adaptive_prefetching" in kwargs: | ||
| val = kwargs["use_experimental_adaptive_prefetching"] | ||
| use_prefetch_reader = ( | ||
| val.lower() in ("true", "1") if isinstance(val, str) else bool(val) | ||
| ) | ||
| else: | ||
| use_prefetch_reader = os.environ.get( | ||
| "USE_EXPERIMENTAL_ADAPTIVE_PREFETCHING", "true" | ||
| ).lower() in ( | ||
| "true", | ||
| "1", | ||
| ) |
There was a problem hiding this comment.
There is a discrepancy in how use_experimental_adaptive_prefetching is resolved when explicitly passed as None in kwargs. In ZonalFile.__init__, an explicit None falls back to the environment variable default. However, in GCSFile.__init__, checking "use_experimental_adaptive_prefetching" in kwargs evaluates to True even if the value is None, which then evaluates bool(None) to False. This disables the prefetcher while keeping cache_type="none", leading to no caching or prefetching at all. Using kwargs.get() to handle None consistently resolves this issue.
| if "use_experimental_adaptive_prefetching" in kwargs: | |
| val = kwargs["use_experimental_adaptive_prefetching"] | |
| use_prefetch_reader = ( | |
| val.lower() in ("true", "1") if isinstance(val, str) else bool(val) | |
| ) | |
| else: | |
| use_prefetch_reader = os.environ.get( | |
| "USE_EXPERIMENTAL_ADAPTIVE_PREFETCHING", "true" | |
| ).lower() in ( | |
| "true", | |
| "1", | |
| ) | |
| val = kwargs.get("use_experimental_adaptive_prefetching") | |
| if val is not None: | |
| use_prefetch_reader = ( | |
| val.lower() in ("true", "1") if isinstance(val, str) else bool(val) | |
| ) | |
| else: | |
| use_prefetch_reader = os.environ.get( | |
| "USE_EXPERIMENTAL_ADAPTIVE_PREFETCHING", "true" | |
| ).lower() in ( | |
| "true", | |
| "1", | |
| ) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #992 +/- ##
==========================================
+ Coverage 89.68% 89.98% +0.29%
==========================================
Files 16 16
Lines 3579 3604 +25
==========================================
+ Hits 3210 3243 +33
+ Misses 369 361 -8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
No description provided.