feat: Support cloud storage in `scan_csv` #16674

nameexhaustion · 2024-06-03T08:41:03Z

This adds support for cloud storage in scan_csv, using a local cache to avoid duplicates downloads. Cached files are removed if they are not accessed within an hour (configurable with POLARS_FILE_CACHE_TTL).

codspeed-hq · 2024-06-03T08:58:14Z

CodSpeed Performance Report

Merging #16674 will not alter performance

_{Comparing nameexhaustion:file-cache (b131a2b) with main (616b2e0)}

Summary

✅ 37 untouched benchmarks

codecov · 2024-06-04T07:21:52Z

Codecov Report

Attention: Patch coverage is 68.42601% with 335 lines in your changes missing coverage. Please review.

Project coverage is 81.36%. Comparing base (ddf8126) to head (b131a2b).
Report is 6 commits behind head on main.

Files	Patch %	Lines
crates/polars-io/src/file_cache/eviction.rs	40.00%	87 Missing ⚠️
crates/polars-io/src/file_cache/entry.rs	74.61%	66 Missing ⚠️
crates/polars-io/src/file_cache/cache_lock.rs	72.13%	34 Missing ⚠️
crates/polars-io/src/file_cache/utils.rs	64.70%	30 Missing ⚠️
crates/polars-io/src/file_cache/file_fetcher.rs	56.92%	28 Missing ⚠️
crates/polars-io/src/cloud/polars_object_store.rs	0.00%	25 Missing ⚠️
crates/polars-io/src/file_cache/metadata.rs	54.34%	21 Missing ⚠️
crates/polars-io/src/file_cache/cache.rs	81.48%	20 Missing ⚠️
py-polars/src/lazyframe/mod.rs	70.83%	7 Missing ⚠️
crates/polars-io/src/pl_async.rs	0.00%	6 Missing ⚠️
... and 4 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16674      +/-   ##
==========================================
- Coverage   81.45%   81.36%   -0.10%     
==========================================
  Files        1413     1421       +8     
  Lines      186096   187205    +1109     
  Branches     2776     2777       +1     
==========================================
+ Hits       151588   152321     +733     
- Misses      33988    34363     +375     
- Partials      520      521       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ritchie46 · 2024-06-05T06:27:15Z

crates/polars-io/Cargo.toml

@@ -20,6 +20,7 @@ ahash = { workspace = true }
 arrow = { workspace = true }
 async-trait = { version = "0.1.59", optional = true }
 atoi_simd = { workspace = true, optional = true }
+blake3 = "1.5.1"


Can we make this an optional dependency? Only activated when we activate the caching?

I put them behind a new file_cache feature flag ~~(although maybe we could also just use the cloud feature flag if you want, let me know)~~
*edit: nvm, I think I prefer the new feature flag because it also specifies "dep:blake3", "dep:fs4"

crates/polars-io/Cargo.toml

crates/polars-io/src/file_cache/file_fetcher.rs

crates/polars-io/src/file_cache/cache_lock.rs

ritchie46 · 2024-06-05T07:23:32Z

crates/polars-io/src/file_cache/cache_lock.rs

+pub(super) type GlobalFileCacheGuardExclusive<'a> = RwLockWriteGuard<'a, GlobalLockData>;
+
+#[derive(Clone)]
+struct AccessTracker(Arc<AtomicBool>);


Can we add a comment explaining what this boolean means?

This was a very good point - I've left a comment summarizing its purpose, but how it achieves its purpose would probably take a few paragraphs 😂

ritchie46 · 2024-06-05T07:31:44Z

crates/polars-io/src/file_cache/cache_lock.rs

+                && verbose
+            {
+                eprintln!("unlocked global file cache lockfile");
+            }


Before sleeping, I think we can also set a notify: https://docs.rs/tokio/latest/tokio/sync/struct.Notify.htm;

As I understand it this will not schedule this task until we notify that we have set the locked file first. If that works I think we can set the sleep shorter to 200 ms or something like that.

Thanks for the tip - I've added the notify layer for the background task to await on, which should make it more efficient.

For the delay I'd prefer to avoid making it too little - the background task gets notified the moment we acquire a lock, so it runs during the entire duration for which we hold the lock (i.e. while we are downloading files).

ritchie46 · 2024-06-05T07:36:01Z

crates/polars-io/src/file_cache/entry.rs

+            self.uri_hash.as_bytes(),
+            remote_metadata.last_modified,
+        );
+        let _ = std::fs::remove_file(data_file_path);


This will block until a new file is downloaded right?

hmm, not exactly..- this was for in case there was a partial download from an aborted process. I've also left a comment above it.

ritchie46 · 2024-06-05T07:39:40Z

crates/polars-io/src/file_cache/eviction.rs

+impl EvictionManager {
+    pub(super) fn run_in_background(mut self) {
+        let verbose = config::verbose();
+        let sleep_interval = std::cmp::max(self.limit_since_last_access.as_secs() / 4, 7);


Can we set this higher to 60 seconds or so? Doesn't have to run often I think?

updated to 60 secs

ritchie46 · 2024-06-05T07:41:06Z

Cool stuff @nameexhaustion. I have left some comments/ questions. But really fun to review this.

…tatic FILE_CACHE_PREFIX`

s-banach · 2024-06-05T14:15:39Z

Closes #13115.

s-banach · 2024-06-05T14:20:19Z

Can you explain POLARS_FILE_CACHE_TTL?
I'm assuming that files are only cached within the context of a single query.
The name "TTL" makes it sound like, if I rerun scan_csv, I'll keep getting an hour-old version of the file I'm trying to scan.

nameexhaustion · 2024-06-06T07:53:31Z

Closes #13115.

Thanks for the triage!

Can you explain POLARS_FILE_CACHE_TTL? I'm assuming that files are only cached within the context of a single query. The name "TTL" makes it sound like, if I rerun scan_csv, I'll keep getting an hour-old version of the file I'm trying to scan.

A query will always check to ensure it has the latest file at the start of execution (i.e. collect()). POLARS_FILE_CACHE_TTL is for configuring how long a cached file is kept on disk based on the time it was last accessed (in seconds).

github-actions bot added the internal An internal refactor or improvement label Jun 3, 2024

nameexhaustion force-pushed the file-cache branch 2 times, most recently from 7491915 to aa6f347 Compare June 4, 2024 06:27

nameexhaustion added 9 commits June 5, 2024 15:20

c

aba8b12

fix test and features

07bfbfd

fix features

cdce48c

fix features

addcfcd

fix features

d0635d0

try ignore wasm

f20c201

fix features

4d9aba1

try wasm

36bb536

add eviction logic

b8b2b11

nameexhaustion force-pushed the file-cache branch from e033eda to b8b2b11 Compare June 5, 2024 05:21

nameexhaustion changed the title ~~test file cache~~ feat: Support cloud storage in scan_csv Jun 5, 2024

github-actions bot added enhancement New feature or an improvement of an existing feature python Related to Python Polars rust Related to Rust Polars labels Jun 5, 2024

nameexhaustion marked this pull request as ready for review June 5, 2024 05:35

nameexhaustion requested review from ritchie46, stinodego, c-peters, alexander-beedie, MarcoGorelli, reswqa and orlp as code owners June 5, 2024 05:35

nameexhaustion removed the internal An internal refactor or improvement label Jun 5, 2024

#[cfg(feature = "async")] -> #[cfg(feature = "cloud")]

793930d

ritchie46 reviewed Jun 5, 2024

View reviewed changes

nameexhaustion added 4 commits June 5, 2024 19:03

introduce file_cache feature, make blake3, fs4 optional

96bb6a9

refactor + add comments to file_fetcher

741e7a5

consolidate path-finding to static POLARS_TEMP_DIR_BASE_PATH and `s…

9c202c6

…tatic FILE_CACHE_PREFIX`

increase minimum sleep interval for gc to 60 secs

312056f

nameexhaustion marked this pull request as draft June 5, 2024 09:12

nameexhaustion added 6 commits June 5, 2024 19:25

fix directory init

f10c04f

code style

da40630

refactor + comment

7392d2b

use notify in background unlock, and add comments

3ca891c

inaccuracy in try_unlock

62afca6

nit

b131a2b

nameexhaustion marked this pull request as ready for review June 5, 2024 11:47

ritchie46 approved these changes Jun 6, 2024

View reviewed changes

ritchie46 merged commit a7f9c8d into pola-rs:main Jun 6, 2024
27 checks passed

nameexhaustion deleted the file-cache branch June 6, 2024 08:35

c-peters added the accepted Ready for implementation label Jun 9, 2024

c-peters assigned nameexhaustion Jun 9, 2024

Wouittone pushed a commit to Wouittone/polars that referenced this pull request Jun 22, 2024

feat: Support cloud storage in scan_csv (pola-rs#16674)

ebd37e5

nameexhaustion mentioned this pull request Jul 8, 2024

scan_csv does not work on s3 path while read_csv works #13115

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support cloud storage in `scan_csv` #16674

feat: Support cloud storage in `scan_csv` #16674

nameexhaustion commented Jun 3, 2024 •

edited

Loading

codspeed-hq bot commented Jun 3, 2024 •

edited

Loading

codecov bot commented Jun 4, 2024 •

edited

Loading

ritchie46 Jun 5, 2024

nameexhaustion Jun 5, 2024 •

edited

Loading

ritchie46 Jun 5, 2024

nameexhaustion Jun 5, 2024

ritchie46 Jun 5, 2024

nameexhaustion Jun 5, 2024 •

edited

Loading

ritchie46 Jun 5, 2024

nameexhaustion Jun 5, 2024

ritchie46 Jun 5, 2024

nameexhaustion Jun 5, 2024

ritchie46 commented Jun 5, 2024

s-banach commented Jun 5, 2024 •

edited

Loading

s-banach commented Jun 5, 2024

nameexhaustion commented Jun 6, 2024 •

edited

Loading

feat: Support cloud storage in scan_csv #16674

feat: Support cloud storage in scan_csv #16674

Conversation

nameexhaustion commented Jun 3, 2024 • edited Loading

codspeed-hq bot commented Jun 3, 2024 • edited Loading

Merging #16674 will not alter performance

Summary

codecov bot commented Jun 4, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

nameexhaustion Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nameexhaustion Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ritchie46 commented Jun 5, 2024

s-banach commented Jun 5, 2024 • edited Loading

s-banach commented Jun 5, 2024

nameexhaustion commented Jun 6, 2024 • edited Loading

feat: Support cloud storage in `scan_csv` #16674

feat: Support cloud storage in `scan_csv` #16674

nameexhaustion commented Jun 3, 2024 •

edited

Loading

codspeed-hq bot commented Jun 3, 2024 •

edited

Loading

codecov bot commented Jun 4, 2024 •

edited

Loading

nameexhaustion Jun 5, 2024 •

edited

Loading

nameexhaustion Jun 5, 2024 •

edited

Loading

s-banach commented Jun 5, 2024 •

edited

Loading

nameexhaustion commented Jun 6, 2024 •

edited

Loading