feat(raw): make dataset concurrency budget dynamic with process-local… - #871
feat(raw): make dataset concurrency budget dynamic with process-local…#871hillhack wants to merge 2 commits into
Conversation
… EMA feedback and env overrides
|
Hey @hillhack. What's your use case ? Also, could you share some benchmarks on different envs ? |
Thanks for taking a look. While auditing Because network speeds vary wildly across environments:
Benchmark ResultsEnvironment: Lightning AI Studio (4× L4 GPUs, AWS S3
Let me know if you have any questions or feedback! |
tchaton
left a comment
There was a problem hiding this comment.
Review
Direction is right (measure, then adapt), but I would not merge as-is. The feedback loop can raise concurrency for the wrong reason, cannot actually drop below today’s floor, and the ImageNet-sized path never measures bandwidth.
What works
- Process-local, pickleable tracker with a lock and
__getstate__/__setstate__fits DataLoader fork/spawn. - Splitting “small GET → latency” vs “large GET → bandwidth” is the right idea.
- Warm-up before swapping defaults, plus env overrides, is a reasonable control surface.
- Tests cover EMA math, pickling, the 5-sample gate, and a few env knobs.
Blocking: the model does not do what the PR / #870 claim
1. ImageNet-sized files never update bandwidth.
Latency observations stop at 256 KiB; bandwidth starts at 1 MiB. Typical ~200 KB JPEGs only move request_latency_s_ema. After warm-up, obs_bps stays None and the code still uses the hardcoded 100 MB/s.
The PR formula
C = max(Bandwidth / Median File Size, Bandwidth × Latency)
is also not what the code does. It is still (bps × 0.5s) // median vs assumed_rate × latency (LITDATA_ASSUMED_REQUEST_RATE / 6000 is unchanged).
2. Small-GET “latency” includes transfer time, so slowness increases concurrency.
_fetch_bytes records full wall time. Little’s-law is 6000 * obs_lat (rate is still assumed). A slow 200 KB GET looks like a large RTT → budget walks toward 512. That is the opposite of the 429 / socket-saturation story in #870.
That also explains the posted bench better than “EMA bandwidth”:
| workers | static budget 256 | if latency-inflated → 512 |
|---|---|---|
| w=8 | 32 permits/worker | 64 — already saturated, ~0% |
| w=24 | max(8, 256//24) = 10 |
512//24 = 21 — matches +58% |
So the win is likely “more slots at high num_workers”, not measured NIC share. Worth a before/after of permit counts, not only ips.
3. The floor makes “scale down on slow networks” false.
test_concurrency_budget_high_and_low_bandwidth_adaptation asserts the 5s / 10 MiB case still budgets 32. Realized aggregate is also max(budget, 8 × num_workers), so w=24 is already 192 in-flight before any EMA. Constrained EKS will not get fewer sockets from this PR.
4. Measurement quality is weak on the instrumented paths.
- Ranged
gather: wall time of parallel ranges over full size → bandwidth overstated. - Hedged GET: clock includes hedge wait / winner, not one transfer.
- Large-object bps subtracts latency EMA (or the 40 ms default) with
max(0.001, dur - lat). A fast 1 MiB GET becomes ~1 GB/s and the EMA spikes. - Mid-size objects (256 KiB–1 MiB) increment
sample_countbut update neither EMA, then unlock “empirical” mode with no empirical bps.
Other issues
_cached_permitsis no longer “once per process”. Invalidating on sample 5 and every 10th sample is fine, but_effective_download_permits’s docstring is now wrong. Replacing theasyncio.Semaphoredoes not shrink in-flight work already holding the old one; you can overshoot the new budget.sample_countis read unlocked in_record_download_observation(prev_count = self._bandwidth_tracker.sample_count). Use the return ofget_metrics()/ the lock._WARMUP_SAMPLE_THRESHOLD = 5is in the PR text only. The5and% 10are magic numbers in three places.- Drive-by:
litdata-raw-pool→asyncio_litdata-raw-poolis unrelated; please drop it. - Env surface: several new
LITDATA_*knobs, no docs, no validation (floor > cap, zeros, negatives).LITDATA_ASSUMED_REQUEST_RATEis wired but not listed in the PR body. - Tests lock in the floor-32 “low bandwidth” behavior and the 0.001s → 500-slot jump. They do not cover the 256 KiB–1 MiB gap, hedge/range timing, or semaphore refresh.
- Changelog not updated (this repo usually does).
Suggested direction
- Keep static defaults until you have enough observations of the right class (N large GETs for bps, N small GETs for RTT), not “any 5 completions”.
- For small objects, estimate RTT as
max(epsilon, duration - size/bps_ema)once bps exists; do not feed full GET time into Little’s law. - Decide explicitly whether the floor of 32 and 8 × workers still apply when measured bps is low. If #870 is the goal, the floor has to move or the 429 case is unchanged.
- Measure one stream (single range or unhedged GET), not parallel/hedged wall clocks.
- Log when the budget changes (you already log on semaphore create) with
bps_ema,lat_ema,samples,budgetso the next bench is interpretable.
Please revise and add a bench that reports concurrency/permits and a slow-link or rate-limited case, not only Studio L4 + S3 val.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #871 +/- ##
====================================
Coverage 81% 81%
====================================
Files 56 56
Lines 9303 9410 +107
====================================
+ Hits 7563 7663 +100
- Misses 1740 1747 +7 🚀 New features to boost your workflow:
|
|
Thanks for the detailed review! I’ve gone through the feedback and implemented the main concurrency-control, bandwidth-tracking, and class-gated observation fixes. 1. Latency was increasing concurrency — Fixed Problem: The previous Little’s Law calculation used observed latency, so higher latency increased the concurrency budget and created a positive feedback loop. Fix: The Little’s Law baseline now uses a fixed target latency: Observed latency is now used strictly as a congestion signal. When reduces the budget instead of increasing it. 2. ImageNet-sized files were not updating bandwidth — Fixed Problem: Bandwidth observations previously required files Fix: The bandwidth observation threshold is now 64 KiB:
This allows ImageNet-sized files to contribute directly to the empirical bandwidth estimate. 3. Single global counter unlocked metrics prematurely — Fixed Problem: A single global Fix:
Static defaults are retained independently for bandwidth and latency until their respective class sample thresholds are met. 4. The fixed floor prevented sufficient scale-down — Fixed Problem: The existing floor of 32 permits prevented the controller from reducing concurrency sufficiently under low-bandwidth/high-latency conditions. Fix: The floor can now adapt down to 1 when high-confidence empirical evidence across both metric classes indicates congestion:
The default floor is retained when evidence is insufficient. 5. Worker-level limits could override the aggregate budget — Fixed Problem: Per-worker minimums could result in effective concurrency of Fix: The aggregate budget is now authoritative: When the aggregate budget is smaller than the number of workers, permits are distributed only up to the available budget rather than forcing every worker to receive a minimum permit. 6. Artificial timing clamp distorted bandwidth estimates — Fixed Problem: The previous Fix: The artificial clamp has been removed. Invalid observations (
7. Added explicit concurrency safety bounds The final budget is now constrained by the effective floor and the maximum concurrency cap of 512: VerificationAll 66 tests in All pre-commit checks, Ruff linters, and formatters pass cleanly. Follow-upI’ve focused this iteration on the core feedback-loop, bandwidth-observation, and sample-gating fixes. I’ll review and address the remaining measurement, semaphore/lifecycle, synchronization, configuration, and benchmarking concerns in a follow-up iteration. |
723785b to
d4399f6
Compare
…worker allocation
d4399f6 to
602f181
Compare
fix: #870
Key Changes
1. Added Process-Local EMA Tracker
Implemented a thread-safe, pickleable
BandwidthTrackerinraw/dataset.py.≥ 1 MiB).< 256 KB).α = 0.2.2. Added Empirical Warm-up
Added
_WARMUP_SAMPLE_THRESHOLD = 5.3. Added Download Telemetry
Instrumented all
CacheManagerdownload paths:adownload_file_fetch_bytes_fetch_ranges_hedgedEvery completed download now feeds its observed latency and payload size back into
BandwidthTracker.4. Dynamic Concurrency Budget
Updated
_aggregate_concurrency_budgetto use measured bandwidth and latency:The result is clamped between 32 and 512.
This allows concurrency to automatically scale down on constrained networks and up on high-performance networks.
5. Added Environment Overrides
Added configurable environment variables:
LITDATA_ASSUMED_BANDWIDTH_BPSLITDATA_ASSUMED_REQUEST_LATENCY_SLITDATA_AGGREGATE_CONCURRENCY_BUDGET_CAPLITDATA_AGGREGATE_CONCURRENCY_BUDGET_FLOORLITDATA_SINGLE_PROCESS_CONCURRENCY_CAPThis provides administrators with manual control without modifying the code.
6. Added Unit Tests
Added coverage in
tests/raw/test_dataset.pyfor:BandwidthTrackerpicklingHow This Fixes the Problem
Self-tuning: Replaces static network assumptions with empirical measurements.
Slow/shared networks: Lower measured bandwidth automatically reduces concurrency, helping avoid socket exhaustion, timeouts, and HTTP 429 throttling.
Fast networks: Higher measured bandwidth increases concurrency (up to 512), improving NIC utilization and reducing GPU data starvation.
Result:
StreamingRawDatasetnow adapts its download concurrency to the actual runtime network and storage characteristics.