Azure-sas-presigned#5
Merged
Merged
Conversation
parse_src handed Azure blob URLs straight to object_store::parse_url_opts, which drops the SAS query string and falls through to the managed-identity credential chain, hanging on the IMDS endpoint. Detect a pre-signed Azure blob/dfs https URL (host under *.blob/dfs.core. windows.net carrying a sig= query) and inject the query as an azure_storage_sas_key option so the store signs requests with the token and skips the credential chain. The token goes last so a URL-embedded SAS wins over an ambient AZURE_STORAGE_SAS_KEY env var. Add unit tests for the detection and an ignored network test that reads an MPC-signed HLS S30 COG window.
Add a "When it helps" section: cptkirk's lever is I/O concurrency via band-subset reads, which benefits multi-band/embedding rasters. Single-band assets see little gain over GDAL's /vsicurl.
object_store routes a plain https URL to its HttpStore and drops the query string, discarding the credential of a pre-signed URL (Huawei OBS / S3 V2 AccessKeyId+Signature, S3 V4 X-Amz-Signature, GCS X-Goog-Signature). Detect these in open_reader and read them with SignedHttpReader, which keeps the full URL and issues only ranged GETs (never HEAD, which OBS rejects). SignedHttpReader overrides get_byte_ranges to coalesce a planar tile's scattered band-plane ranges into one multipart/byteranges request: N round-trips collapse to one over a single connection. This is decisive on high-latency HTTP/1.1 stores (OBS): ck_warp full read 82s->24s, AOI 12s->7s. Falls back to bounded-concurrent single-range requests when the server ignores multi-range, detecting a 200 (whole object) from the status before reading the body so a non-cooperating server never triggers a whole-file download. Built on reqwest 0.12 + ring/rustls (reuses object_store's stack) rather than async-tiff's ReqwestReader (reqwest 0.13 pulls aws-lc-rs + cmake). Reader unified to Arc<dyn AsyncFileReader>. Adds bench_warp_vs_curl.R.
ck_stack fetches a window from N sources through one global io pool and writes them stacked as separate bands (the gdalbuildvrt -separate analogue), versus ck_warp's mosaic of multiple sources. ck_stack_read returns the array. ck_batch lifts this to a list of groups: one pooled fetch across every asset in every group (maximum network saturation), with structure-preserving output -- stack=FALSE writes one file per band (return mirrors the nested input), stack=TRUE writes one band-separated file per group. dst takes an explicit path structure or a template expanded from group/band names. Single AOI across the batch; non-overlapping sources return NA. Refactors .stack_assemble out of the stack engine so all three share the fetch/stage/stack/materialise path. Offline tests via local fixtures.
`r` may now be a single method (applied to all) or, matching the input structure, one method per source -- e.g. "near" for a mask band and "bilinear" for reflectance. ck_stack/ck_stack_read take a length-1 or length(src) vector; ck_batch takes length-1, a flat per-asset vector, or a list mirroring src. The per-source method is threaded through the shared .stack_assemble warp branch (it only applies on the warp path; a native-window copy ignores resampling).
ck_batch warps many small windows one at a time, where per-warp GDAL thread spin-up costs more than it saves. Benchmarked ~8x slower at "ALL_CPUS" than at 1 for this many-small-files pattern (47.8s vs 6.1s for 204 reprojected assets), so default to 1. ck_warp/ck_stack keep ALL_CPUS (larger single warps benefit).
…ori) The fetch already runs in one saturating pool, but the per-output warp+write ran serially -- the bottleneck on large many-files batches. ck_batch can now warp and write the outputs across ambient mirai daemons: each fetched window buffer is shared into OS shared memory with mori (serialised to daemons as a tiny reference, not a per-worker copy), and each daemon warps single-threaded. Controlled by `parallel`: NULL (default) auto-enables when mirai + mori are installed and daemons are running; TRUE requires them; FALSE forces serial. cptkirk uses the ambient mirai::daemons() pool and never spawns/tears down daemons. Falls back to the serial path otherwise; output is identical either way. mirai + mori added to Suggests. Gated test verifies the parallel path matches the serial output bit-for-bit.
Adds a streaming fetch session in Rust (cog_sources_open / cog_fetch_stream_begin / cog_fetch_take): open every source once concurrently, then push each fetched window back to R as it completes through one buffer_unordered pool. ck_batch now drives it instead of meta-then-fetch: it opens once (no double header read), and assembles each output as soon as its window(s) arrive -- serially or dispatched to a daemon -- so the warp/write overlaps the fetch and peak memory is bounded (windows are dispatched and freed on arrival rather than all held). Measured: open-once streaming fetch 18.1s vs 29.1s double-open (204 windows, -38%); end-to-end ck_batch ~22s, now fetch-bound with the warp hidden behind the fetch (serial ~= parallel). Output identical to the prior path; all tests pass.
Every acquisition of one MGRS tile shares the same grid, but .plan_from_meta ran a PROJ transform_bounds per source -- the dominant prep cost (2.7s of 3.0s planning for 204 same-tile assets). Compute the plan once per unique grid signature (geotransform + crs + level dims) and reuse it, attaching each source's URL. Prep drops ~2.7s on a single-tile batch; planning is now negligible next to the open phase.
sanitise = TRUE (default) still opens every source to plan/verify its grid. sanitise = FALSE trusts that all sources share one grid: it plans the window from a SINGLE source, then folds every other open into the fetch stream (cog_stream_fetch_urls_begin opens each source inside its own fetch task) rather than opening all up front. The per-source header read no longer forms a barrier -- opens overlap the fetch. Off-grid sources fail their fetch (-> NA) rather than open-all overhead, but identical-size off-grid tiles are the caller's responsibility under sanitise = FALSE. ~2.4s faster on a 204-asset single-tile batch (opens overlapped, not eliminated). Output identical to the default; test confirms.
The open was connect-bound: cptkirk built a separate object_store client per source, so opening N same-host COGs paid N TLS handshakes with no keep-alive reuse (curl/GDAL share connections). Measured: 50 same-host header reads were 5.5s with separate connections vs 1.1s reused. Add OpenCache (source.rs): object_store stores cached by store identity (scheme/host/container/credential) and one shared reqwest client for SignedHttpReader, so same-host sources share a connection pool. open_reader_cached builds readers from it; open_tiff_from_reader reads metadata over the shared pool. cog_sources_open uses it -> cog_sources_open on 204 MPC assets dropped from 6.6s to 1.3s. (MPC's SAS is container-scoped, so one store serves every asset.) Because the open is now cheap for both paths, the trusted fast path no longer needs to fold opens into the fetch stream: ck_batch routes both sanitise modes through cog_sources_open + cog_fetch_stream_begin, with sanitise = FALSE only changing planning (one grid, from a single source). Removes cog_stream_fetch_urls_begin. Also fixes stack = TRUE band order under streaming (windows arrive in completion order; bands are now reordered by source position). Verified on the live MPC path: trusted output matches default bit-for-bit.
The connection-reuse and grid-signature plan-cache wins built for ck_batch lived only in its open path; ck_warp / ck_read / warp_remote / ck_stack / cog_info still used the old per-source-client opens (one TLS handshake per source) and an uncached per-source PROJ transform. Rust: add `open_all_cached` -- open every source once over one OpenCache connection pool per host, then read headers concurrently. Route cog_meta_many, cog_fetch_windows_raw and cog_sources_open through it. So every multi-source entry point pays ~one handshake per host, not one per source. R: extract `.plan_sources` (plan cached by grid signature) and use it in both .warp_collect (ck_warp/ck_read/warp_remote/ck_stack) and ck_batch, so the PROJ transform_bounds runs once per unique grid, not per source. Live MPC: cog_info x156 now 0.8s (5 ms/src), matching the reused rate. All 239 tests pass.
ck_batch is fetch-bound: the streaming drain loop already warps each window the moment it lands, overlapping the fetches still in flight, so the per-output warp hides behind the network. Measured serial vs parallel (8 daemons + mori) on 204 assets at 1024x1024: 20.9s vs 21.7s (NONE), 21.6s vs 20.1s (DEFLATE) -- within network noise. Parallelising the warp has nothing to bite on when the warp isn't the bottleneck, which is structural for a remote-COG warper. Drop the `parallel` arg, `.batch_use_parallel`, the mirai::mirai/mori::share dispatch branch and the everywhere() daemon-load; `.batch_stream_run` now always assembles inline. Remove mirai/mori from Suggests and the gated parallel test. The streamed warp-on-arrival (cog_fetch_take drain loop) is retained -- that is the overlap that delivers the measured time.
ck_stack / ck_stack_read / ck_batch were exported but absent from the package ?cptkirk overview and the pkgdown reference. Add a 'Stacking and batch' section to both, and note the shared per-host connection pool + warp-on-arrival in the 'How it works' overview. pkgdown::check_pkgdown() now finds no missing topics.
- H1: validate parallel window-vector lengths and idx range in cog_fetch_windows_raw / cog_fetch_stream_begin. Out-of-range/mismatched input indexed a raw Vec -> panic across the FFI boundary -> R session abort (extendr cannot catch panics). Now returns a KirkError. - H2: plan_tile now rejects out-of-range bands (as fetch_decoded already did); the chunky blit slices by band index and would otherwise panic out of bounds. - H3: ck_batch no longer silently turns fetch FAILURES into NA. Failed fetches are collected and surfaced via cli_warn (non-overlap stays silent, as documented); @return updated. stack=TRUE still NA-s the whole group on a band failure, now with a warning. - H4: drop the undeclared `fs` dependency (R CMD check WARNING); the ck_batch default dst uses base tempfile(fileext = ".tif"). - M1: .plan_sources refreshes per-source nodata on a grid-signature cache hit, so same-grid sources with different nodata no longer inherit the first's (drove wrong staging INIT_DEST/masking).
- M2: sanitise=FALSE now verifies all sources share the first's grid signature (headers are already in hand from cog_sources_open) and aborts on a mismatch, removing the silent-wrong-pixels risk for same-size off-grid tiles; it also applies per-source nodata. Grid signature extracted to .grid_sig. - M3: a dst template that expands to duplicate paths (duplicate group/band names) now errors instead of silently overwriting. - M4: pre-signed HTTP clients get connect/read timeouts (build_http_client), so a stalled source fails instead of hanging the blocking R consumer forever. - M5: streaming fetch uses a BOUNDED tokio channel (awaited sends) -> real backpressure, peak memory ~io windows instead of the whole job. - M6: the detached fetch task is aborted when the FetchSession is dropped, so abandoning a stream stops fetching instead of draining into a closed channel. - M7: a rejected multi-range request falls back to per-range fetches instead of failing the whole read. - M8: empty byte ranges (sparse COG tiles) short-circuit, avoiding range.end-1 underflow on the pre-signed path. - Clippy: 0 warnings (split_once; documented allow on the two load-bearing/clear cases).
Cover the previously-untested paths: non-overlapping source -> NA; stack=TRUE band order preserved when a middle band is missing; sanitise=FALSE aborts on mixed grids; dst template collision on duplicate names errors; .plan_sources assigns each same-grid source its own nodata (grid-cache hit).
Reinstated the per-output warp/write dispatch across an ambient mirai daemon pool (window buffers shared zero-copy via mori). The earlier removal was based on a fetch-bound benchmark; on slow links or heavy/large warps the inline warp in the drain loop backs up behind the fetch, where offloading to daemons is a real win. Control is auto-detect only -- no `parallel` argument. .batch_use_parallel() returns TRUE iff mirai + mori are installed and daemons are running; start them (mirai::daemons(n)) to enable, stop them (daemons(0)) to disable. The streamed fetch is always cptkirk's single pool. mirai/mori back in Suggests; gated test restored (now asserts the auto-selected daemon path matches serial).
The daemon warp dispatch named the internal helper as cptkirk:::.stack_assemble, tripping the "package uses ::: for its own objects" NOTE that CI rejects. Pass .stack_assemble to mirai::mirai() as a value (`asm`) instead: its environment is cptkirk's namespace, restored on the daemon (which loads the package via everywhere()), so the call needs no namespace qualifier. Verified the daemon path still matches serial.
R CMD check --as-cran flagged an undeclared pkgload import in tests (the gated daemon test uses pkgload for dev-mode loading) and non-standard top-level files. Add pkgload to Suggests; .Rbuildignore the untracked benchmark scratch.
pkgload was only used by the gated daemon test to push the load_all build into mirai daemons -- not by the package itself. Remove it: the daemons load cptkirk via ck_batch's own everywhere(library(cptkirk)), so the test needs only an INSTALLED package; skip it under devtools::load_all (detected via DEVTOOLS_LOAD, no dependency). Clears the --as-cran 'unstated pkgload import in tests' WARNING.
Tests assume the package is installed (as R CMD check / CI run them); the DEVTOOLS_LOAD check was a non-standard wart. Plain skip_on_cran + skip_if_not_installed guards only.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Title: Pre-signed remote reads + pooled stacking/batch API + connection reuse
Summary
This branch extends cptkirk from a single-raster remote warper into a saturating multi-source engine, and makes the whole API share one fast, connection-reusing fetch path. Three arcs:
All changes are additive; no public function was removed or had its signature broken.
What's new
Pre-signed remote readers
Stacking & batch API
- ck_stack / ck_stacm N sources andwrite/return them as separate bands of one output (the gdalbuildvrt-separate analogue,
Performance
Hardening (from an internal review)
A three-agent review (Rust soundness / R API / build) ran before mefindings fixed: