feat(presto/bench): config buffer tuning and benchmark robustness fixes - #390
feat(presto/bench): config buffer tuning and benchmark robustness fixes#390misiugodfrey wants to merge 9 commits into
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
4d119ee to
4fd0ca2
Compare
Based on kjmph's bench33 POC patch. - run_benchmark.sh: add --maxfail=1 when --profile is set; add PRESTO_BENCHMARK_KEEP_VENV escape hatch to skip venv teardown - common_functions.sh: replace jq-based worker registration wait with python3 JSON parse; fix while-loop so it actually retries correctly - common_fixtures.py: fix error handling for non-Presto exceptions in failed_queries_dict; move REST metrics collection outside profiler interval; add PRESTO_SESSION_PROPERTIES env var support - hive.properties: comment out SOFT_AFFINITY in base template; keep in CPU-specific override only - Config overrides: exchange.max-response-size=64MB for CPU coordinator; lower buffer defaults to 32MB for CPU workers; add local-exchange and largest-size-class-pages knobs; tune GPU exchange/sink buffer sizes - .gitignore: add result_dir_*/ and latest_result_dir.txt
4fd0ca2 to
47b0a20
Compare
|
|
||
| # CPU UCX row pages and local exchange were tuned together. The generator | ||
| # reapplies these as overridable CPU_* values on every run. | ||
| local-exchange.max-buffer-size=536870912 |
There was a problem hiding this comment.
Note that these values can't have units associated with them (MB for instance), so we need to leave this as a byte value.
| while true; do | ||
| if curl -s -f -o node_response.json "${COORDINATOR_URL}/v1/node"; then | ||
| node_count=$(python3 -c \ | ||
| 'import json, sys; print(len(json.load(open(sys.argv[1]))))' \ |
There was a problem hiding this comment.
replaced jq with python parsing since jq is not installed in the cluster/coordinator image by default.
There was a problem hiding this comment.
Could we complete the jq dependency removal in the Slurm launcher as part of this change? presto/slurm/presto-nvl72/functions.sh still stages a static jq binary at lines 550–570 and prepends its directory to PATH at line 577. Thus, a clean run still downloads jq from GitHub before launching the benchmark. An environment without access can still fail even though jq is no longer used.
Hence, can we remove the jq download block and remove /workspace/.cache/bin PATH prefix from run_coord_image? The source patch, bench33.patch removed both pieces.
kjmph
left a comment
kjmph
left a comment
There was a problem hiding this comment.
In addition to the inline review comments, I see a few broader or lower-priority items:
-
The PR description says
result_dir_<jobid>/andlatest_result_dir.txtwere verified, but that behavior does not appear to be implemented by this PR's current head. The diff only adds related.gitignoreentries. It looks like some validation may have included stacked changes fromMultiProfileorbench33.patch. Could we rerun the stated validation from a clean checkout of this exact PR head, then either include the missing dependencies or update the checklist? -
--iterations=0 --metricsreaches the metrics collection path without ever assigning a query cursor. It would be good to reject non-positive iteration counts during argument validation. -
PRESTO_SESSION_PROPERTIEScurrently accepts an empty property name such as=value. Could we reject that locally and report which entry is malformed? -
The CPU template comments advertise
CPU_*overrides for the local-exchange and page-size settings, but the corresponding generator logic is not present in this PR. We should either port that plumbing or revise the comments so they describe what this patch supports.
The inline comments contain the changes I consider most important. My main overall feedback is ensuring that the final patch is self-contained.
| # large distributed shuffles. | ||
| exchange.max-buffer-size=512MB | ||
| sink.max-buffer-size=512MB | ||
| exchange.max-response-size=64MB |
There was a problem hiding this comment.
Could we pair this exchange.max-response-size increase with the coordinator’s HTTP content limit? With 64MB, Presto can request roughly 48 MB after its 75% encoding allowance, while exchange.http-client.max-content-length remains at its 32 MB default. I received errors on SF=30K when we went over this limit.
The source benchmark attempted to set the HTTP limit to 128 MB through --config-overrides, but that helper was replace-only for coordinator properties. Because this key was absent, the override was silently applied only to native workers and never reached the Java coordinator. Now that we have this in the coordinator, we can override in testing. Alternatively, we could also extend --config-overrides to allow new propertise.
I can test once this PR is merged, and if we still have problems, the conservative fix is to use exchange.max-response-size=32MB. Yet, I'm pretty sure we are narrowing down why the prior test hetting exchange.http-client.max-content-length didn't work.
There was a problem hiding this comment.
I'll go with the 128MB pairing for max-content-length for now. Unless you think we should leave both as default and refine it further once we do some explicit testing?
| local-exchange.max-buffer-size=536870912 | ||
| exchange.max-buffer-size=512MB | ||
| sink.max-buffer-size=512MB | ||
| exchange.max-response-size=64MB |
There was a problem hiding this comment.
I think this is a coordinator only setting.. Can we remove this in the GPU and CPU worker properties, so it isn't confusing in the future?
There was a problem hiding this comment.
Removed from both cpu/gpu worker properties. It looks like the workers silently ignore this option
| exchange.max-buffer-size=32MB | ||
| sink.max-buffer-size=32MB | ||
| # Increase max bytes returned per exchange HTTP response to reduce round trips for large shuffles. | ||
| exchange.max-response-size=64MB |
There was a problem hiding this comment.
Similarly, remove this here, as this pre-existed the commit, and I think what confused me.
| ${LOGS}:${vt_cufile_log_dir},\ | ||
| ${LOGS}:${vt_nsys_report_dir}${driver_mounts}${gds_mounts:+,${gds_mounts}}${worker_extra_mounts} \ | ||
| ${LOGS}:${vt_nsys_report_dir},\ | ||
| ${libcudf_cache}:/root/.libcudf${driver_mounts}${gds_mounts:+,${gds_mounts}}${worker_extra_mounts} \ |
There was a problem hiding this comment.
@lamarrr, @bdice, could you weigh in on the intended libcudf cache location here? The relevant cuDF version defaults LIBCUDF_KERNEL_CACHE_PATH to $HOME/.cudf, yes? This mounts the writable directory at /root/.libcudf.
Unless the environment variable is set elsewhere to /root/.libcudf, this mount will not cover cuDF’s default cache path. @lamarrr mentioned that a few different locations are tested, so maybe I'm mistaken.
Yet, I think this mount target should be $HOME/.cudf.. or should the launcher explicitly set LIBCUDF_KERNEL_CACHE_PATH=xx as in PR #398 ?
There was a problem hiding this comment.
I'll go with LIBCUDF_KERNEL_CACHE_PATH for now, but will update if we get further feedback.
| sink.max-buffer-size=1GB | ||
| # Single-worker fallbacks. generate_presto_config.sh raises exchange/sink to | ||
| # 512MB whenever the full cluster has more than one worker. | ||
| exchange.max-buffer-size=32MB |
There was a problem hiding this comment.
Was the corresponding generate_presto_config.sh change intentionally omitted? The comment says these are single-worker fallbacks and that multi-worker clusters are raised to 512MB, but the generator in this PR never modifies exchange.max-buffer-size or sink.max-buffer-size.
Consequently, every worker in a newly generated multi-worker CPU run will use 32MB for both buffers. Before this PR, each worker used 1GB. The source patch, bench33.patch intended 512MB for multi-worker runs. The coordinator also does not receive the intended 512MB values.
Perhaps we port the buffer reconciliation from the bench33.patch after the numbered worker configurations have been created? Use 32MB for a single worker and 512MB for multiple workers? We can apply both properties with replace-or-append to the coordinator and every etc_worker_* configuration.
If that generator locic is intentionally deferred, perhaps we avoid lowering the static worker values in this PR and remove the comments promising behavior that will be present later?
There was a problem hiding this comment.
Most of the generator logic is deferred to the next PR (#391), but some of it leaked over. For now I'll have these switch to the 512MB values by default (the intended multi-worker values) and the subsequent PR can add the single-worker configuration.
| # profiler interval. Otherwise CPU samples after query | ||
| # completion describe metrics collection and worker idling, | ||
| # rather than the engine work the profile is meant to measure. | ||
| if metrics: |
There was a problem hiding this comment.
I think this hunk was ported without the per-iteration profiling structure it depended on in the source patch.
Moving metrics collection outside the iteration loop means only the final iteration's Presto query ID is collected. With the default five iterations, the previous code produced five query-specific
metrics files. Yet, this produces one. If a later iteration fails, metrics are not collected for any earlier successful iteration.
It also does not currently move metrics outside the profiler interval. Profiling starts before the loop at line 133 and stops in the outer finally at line 176, after this call.
I suggest we either port the complete per-iteration profiling structure from the source patch, bench33.patch, or retain the current combined mode while recording every iteration's Presto query ID, stopping and waiting for the profiler after the loop. Then collect metrics for every recorded ID. We should verify that collection begins only after profiling has stopped.
| while true; do | ||
| if curl -s -f -o node_response.json "${COORDINATOR_URL}/v1/node"; then | ||
| node_count=$(python3 -c \ | ||
| 'import json, sys; print(len(json.load(open(sys.argv[1]))))' \ |
There was a problem hiding this comment.
Could we complete the jq dependency removal in the Slurm launcher as part of this change? presto/slurm/presto-nvl72/functions.sh still stages a static jq binary at lines 550–570 and prepends its directory to PATH at line 577. Thus, a clean run still downloads jq from GitHub before launching the benchmark. An environment without access can still fail even though jq is no longer used.
Hence, can we remove the jq download block and remove /workspace/.cache/bin PATH prefix from run_coord_image? The source patch, bench33.patch removed both pieces.
| hive.node-selection-strategy=SOFT_AFFINITY | ||
| # Affinity scheduling is variant-specific. CPU enables soft affinity in its | ||
| # override; GPU can opt in explicitly when placement experiments require it. | ||
| # hive.node-selection-strategy=SOFT_AFFINITY |
There was a problem hiding this comment.
Upon review, could we preserve the existing Java benchmark behavior here? This shared coordinator template is also used by the Java variant. Commenting out SOFT_AFFINITY here and restoring it only in the CPU override changes Java from SOFT_AFFINITY to Presto's default NO_PREFERENCE.
The stated intent is to disable affinity for GPU while retaining it for CPU.. Yet, maybe we want to preserve the pre-PR behavior for Java but adding a Java coordinator override containing hive.node-selection-strategy=SOFT_AFFINITY?
If we are intentionally changing Java, perhaps it should be called out and validated explicitly..
There was a problem hiding this comment.
I'll add it to java for now, although I'm pretty confident that we never confirmed if we want SOFT_AFFINITY for java runs or not. I think it makes sense not to change more behavior than intended - but I also want to flag that adding an explicit option here implies that this option's affect on the java worker is more empirically measured than it is.
| user=user, | ||
| catalog="hive", | ||
| schema=schema, | ||
| session_properties=_session_properties_from_env(), |
There was a problem hiding this comment.
Since this feature is intended for per-run optimizer and execution tuning, could we record the effective session properties in the benchmark results? They are currently passed to the Presto connection but are not included in run_context or the generated configuration snapshot.
Two runs using different PRESTO_SESSION_PROPERTIES can produce materially different plans and timings while appearing to have identical configurations in benchmark_result.json. post_results.py also filters context fields that are not declared in BenchmarkMetadata.
Perhaps we parse the properties once, and pass the resulting map to the connection while storing the same normalized map under a session_properties context field? We can add the corresponding field to BenchmarkMetadata so it survives result posting. Maybe the env var format should also be documented alongside the benchmark options?
There was a problem hiding this comment.
I've added sessions_properties in the benchmark_results.json's context and the format is now documented.
| # Provide a writable directory for libcudf's JIT/RTC cache. The container | ||
| # root filesystem is read-only squashfs, so /root/.libcudf is not writable | ||
| # without this mount. | ||
| local libcudf_cache="${SCRIPT_DIR}/libcudf_cache_${worker_id}" |
There was a problem hiding this comment.
If these per-worker cache directories remain after resolving the cache-path question above, could we keep them out of the working tree status? Each Slurm run creates libcudf_cache_<worker_id> beneath the tracked presto-nvl72 directory, but the current .gitignore covers worker_data* and not these new directories.
Shall we add presto/slurm/presto-nvl72/libcudf_cache_*/ to .gitignore, or place the cache beneath an existing ignored per-worker runtime directory. If the earlier cache-path change removes these separate directories, no additional change is needed.
_session_properties_from_env() calls os.environ.get() but os was not imported, causing NameError on every presto_cursor fixture initialization.
The native worker rejects DataSize suffixes (MB) for this property; express 512MB as raw bytes instead.
…iner The container root is read-only squashfs so /root/.libcudf is not writable, causing libcudf JIT/RTC cache creation to fail. Bind-mount a per-worker host directory to provide the writable path.
…ning # Conflicts: # presto/testing/performance_benchmarks/common_fixtures.py
c28bd27 to
36de78f
Compare
- Remove jq download block and PATH prefix from run_queries; replace remaining jq-based wait_for_workers_to_register with python3 - Add exchange.http-client.max-content-length=128MB to GPU coordinator to pair with exchange.max-response-size=64MB (avoids client rejecting responses it requested at SF30K+) - Remove exchange.max-response-size from GPU and CPU worker configs; native workers do not read this property (not in Configs.h) - Fix libcudf cache mount target: /var/lib/presto/data/libcudf-cache matches LIBCUDF_KERNEL_CACHE_PATH set by PR #398 in the native image - Port full per-iteration profiler structure (PROFILE_ITERATIONS env var, combined and per-iter modes) with CTAS integration; collect metrics per-iteration after each per-iter profiler stop - Add java/etc_coordinator/catalog/hive.properties override to preserve SOFT_AFFINITY for Java benchmarks after it was removed from the shared template - Record PRESTO_SESSION_PROPERTIES in benchmark_result.json context as session_properties; add field to BenchmarkMetadata; document format in _session_properties_from_env docstring; reject empty property names - Reject --iterations < 1 at session start - Set CPU worker exchange/sink buffers to 512MB (safe multi-worker default until generate_presto_config.sh reconciliation lands in #391); remove misleading comment promising generator behavior not in this PR - Remove CPU_* generator promise comment from cpu/etc_worker template
2 participants
Summary
Low-risk correctness fixes and config tuning for Presto native benchmarks. Good first merge of this series.
Based on kjmph's bench33 POC patch.
--maxfail=1when--profileis set (avoids waiting for all queries when one fails during profiling runs); addPRESTO_BENCHMARK_KEEP_VENVescape hatchjq-basedwait_for_worker_node_registrationwithpython3JSON parse; fix broken while-loop that was not actually retrying correctlyfailed_queries_dict; move REST metrics collection outside the profiler interval so profiling overhead doesn't skew results; addPRESTO_SESSION_PROPERTIESenv var for per-run session overridesSOFT_AFFINITYin base template; keep it in the CPU-specific override (overrides/cpu/) only — GPU experiments should opt in explicitlyexchange.max-response-size=64MBfor CPU coordinator; lower CPU worker buffer defaults to 32MB; addlocal-exchange.max-buffer-sizeandlargest-size-class-pages; tune GPU exchange/sink buffer sizesresult_dir_*/andlatest_result_dir.txt(introduced by PR Land @bdice prototype workflows in main for testing #3 of this series)Dependencies
None. This PR is self-contained and should merge first.
Verification
--profile; verify benchmark aborts after the first query failure instead of continuing through all 22 queriespresto/scripts/run_benchmark.shand confirm thePRESTO_BENCHMARK_KEEP_VENV=1escape hatch skips venv teardownwait_for_worker_node_registrationretries correctly when workers are slow to register (previously the loop exited immediately on the first iteration)common_fixtures.pymetrics are collected after the profiler window closes (check thatQueryStatsREST calls don't appear inside the nsys/perf capture interval in logs)./ci/check_style.sh(ruff + codespell) on changed Python files