fix(tuning): author-experience honesty quickfixes (dry-run preview, flag guard, resolver hints)#1172
Merged
Merged
Conversation
…lag guard, resolver hints)
Four independent quickfixes from the 2026-07-12 tuning review (findings R3/N1/N2/N3):
- w1: _extract_ddl_preview did an exact-key lookup into table_tunings, but
benchmark table names are lowercase ("lineitem") while shipped templates key
tables uppercase ("LINEITEM"), so the tuned dry-run preview was always empty.
Added a case-insensitive lookup (mirrors profile_validation.extract_template_
columns) plus a regression test against the real examples/tunings/duckdb/
tpch_tuned.yaml template. Making this observable also surfaced two
co-requisite bugs in the same file/scope, now fixed: _extract_ddl_preview
gated on benchmark.get_tables(), a method no benchmark implements (only
platform adapters do) - switched to get_schema(), the method
_generate_external_schema_sql already uses for the same purpose; and
_build_table_ddl_entry referenced TuningClauses fields (distribution_key/
distribution_style) that don't exist on the dataclass, raising AttributeError
as soon as the lookup fix let it run - now delegates to
TuningClauses.get_inline_clauses() (already used by generate_create_table_ddl).
- w2: dry-run's Platform Optimizations display showed Databricks-only default
fields (e.g. "Databricks Clustering Strategy: z_order") for every platform.
The actual display code lives in benchbox/cli/dryrun.py (out of scope), so
the fix threads platform through DryRunExecutor._extract_tuning_config /
_extract_unified_tuning and gates databricks_clustering_strategy on
platform == "databricks" at the data source.
- w3: --dry-run took a required OUTPUT_DIR string and silently swallowed a
following flag (`--dry-run --non-interactive` created a directory literally
named "--non-interactive"). Added a click callback rejecting values starting
with '-' with a clear error.
- w4: resolve_tuning checked path-existence (Case 3) before the "tuned"
keyword (Case 4), so a local file/dir named "tuned" could shadow the
keyword. Reordered so all keyword checks (notuning/auto/tuned) precede the
path-existence check. Also fixed the invalid-value error hints, which
pointed at "--tuning list" (itself invalid) instead of "benchbox tuning list".
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QTnjb2i44JUL71kmu9sk1t
…ds (review nit) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QTnjb2i44JUL71kmu9sk1t
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9915954bb6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…seline PR #1172's required test job was red on two guardrails: - tests/system/test_module_size_thresholds.py: the --dry-run flag-guard callback added in the previous commit pushed benchbox/cli/commands/run.py to 2962 lines (limit 2945). Relocated the validation logic itself to a new ValidationRules.validate_dry_run_output_dir staticmethod in benchbox/cli/exceptions.py (already imported by run.py, and already home to the analogous validate_output_directory), leaving only a one-line callback= reference in run.py. That reference is an irreducible integration point, so the allowlist limit is bumped by exactly 1 (2945 -> 2946) with a justification comment, per the guard's own documented escape hatch. - tests/uat/test_no_cli_surface_drift.py: flagged benchbox/cli/tuning_resolver.py as an unexpected CLI file change (its baseline is a diff against origin/develop gated by an explicit per-file allowlist). Added tuning_resolver.py (keyword-vs-path reordering, error-hint text fix) and exceptions.py (the relocated callback, added by this commit) to ALLOWED_INTERNAL_CLI_FILES with justification comments. Neither touches any click command/option/argument/group decorator or the tracked commands' signatures, so they were intentionally left out of ALLOWED_HIDDEN_COMPAT_CLI_FILES - the surface-parity check still runs and passes for both, rather than being skipped. linkcheck/spellcheck failures observed on the same run are pre-existing and unrelated: confirmed via the GitHub Actions API that they fail identically, at the same time, on multiple unrelated concurrent PRs (e.g. chore/pr-review-followup-sweep-12) - not something this branch's diff could cause. Left untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QTnjb2i44JUL71kmu9sk1t
joeharris76
pushed a commit
that referenced
this pull request
Jul 16, 2026
Merged origin/develop (PR #1172's module-size allowlist bump to 2946, plus #1174/#1175/#1170/#1177 tuning soundness/explorer work) - merge was conflict-free. The auto-mode warning added in the previous commit pushed benchbox/cli/commands/run.py to 2957 lines, 11 over the just-bumped 2946 limit in tests/system/test_module_size_thresholds.py. Relocated the warning logic to benchbox/cli/tuning_resolver.py (which already owns tuning messaging via display_tuning_resolution) as a new warn_sql_auto_mode() function, and replaced the ~13-line inline block in run.py with a single call. Folded the branch's existing generic "using basic unified config" debug log into the same function (it only ever fires alongside the auto-mode case), which brought run.py to exactly 2946 lines without bumping the allowlist. Added unit coverage for warn_sql_auto_mode in test_tuning_resolution.py: warns on SQL platforms, silent on DataFrame platforms and non-auto modes, respects quiet=True, and always logs the debug line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QTnjb2i44JUL71kmu9sk1t
12 tasks
joeharris76
added a commit
that referenced
this pull request
Jul 16, 2026
…ence; honest auto-mode message (#1178) * docs(tuning): fix env contract, rewrite stale README, document precedence; honest auto-mode message Re-verified the 2026-07-12 tuning review findings (R5) against HEAD (post-#1172 resolver hints/keyword-first fixes) before implementing: - w1: BENCHBOX_TUNING_ENABLED set config key tuning.enabled, which nothing at runtime read except a unit test. Removed both env-var mapping mechanisms (module-level env_mappings dict and the ConfigManager default's environment_overrides entry) from benchbox/cli/config.py, the dead "enabled" default key, and the false "activates tuned runs in CI" doc claims in docs/usage/configuration.md and docs/reference/cli/configuration.md. BENCHBOX_TUNING_CONFIG and BENCHBOX_TUNING_PATH behavior is unchanged. Added a CHANGELOG entry. - w2: rewrote examples/tunings/README.md - the real flag is --tuning (not --tuning-config), the named example scripts don't exist (pointed at the real examples/unified_runner.py and examples/features/tuning_comparison.py instead), file layout matches examples/tunings/<platform>/<benchmark>_tuned.yaml, auto-discovery via --tuning tuned is the primary UX, and dropped the unqualified "2-10x" performance claim. - w3: documented the actual --tuning resolution order in docs/reference/cli/tuning.md (default notuning -> keyword check before path check -> tuned's default_config_file/BENCHBOX_TUNING_CONFIG beats discovery -> BENCHBOX_TUNING_PATH -> cwd-relative examples/ -> cwd -> fallback with warning), the cwd-relative caveat for installed-package users, the tuning list/show/platforms subcommands, and the correct --output default (<platform>_tuning.yaml / DataFrame's <platform>_<profile>_tuning.yaml, not tuning_config.yaml). - w4: --tuning auto now warns explicitly on SQL platforms that smart defaults are DataFrame-only today and the run proceeds with a basic config, instead of silently building an untuned config while claiming smart defaults were applied. Message change only, no behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QTnjb2i44JUL71kmu9sk1t * fix(cli): relocate SQL auto-mode warning to resolver; merge develop Merged origin/develop (PR #1172's module-size allowlist bump to 2946, plus #1174/#1175/#1170/#1177 tuning soundness/explorer work) - merge was conflict-free. The auto-mode warning added in the previous commit pushed benchbox/cli/commands/run.py to 2957 lines, 11 over the just-bumped 2946 limit in tests/system/test_module_size_thresholds.py. Relocated the warning logic to benchbox/cli/tuning_resolver.py (which already owns tuning messaging via display_tuning_resolution) as a new warn_sql_auto_mode() function, and replaced the ~13-line inline block in run.py with a single call. Folded the branch's existing generic "using basic unified config" debug log into the same function (it only ever fires alongside the auto-mode case), which brought run.py to exactly 2946 lines without bumping the allowlist. Added unit coverage for warn_sql_auto_mode in test_tuning_resolution.py: warns on SQL platforms, silent on DataFrame platforms and non-auto modes, respects quiet=True, and always logs the debug line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QTnjb2i44JUL71kmu9sk1t * fix(cli): don't call the SQL --tuning auto fallback config "untuned" warn_sql_auto_mode's warning called the fallback UnifiedTuningConfiguration() "basic (untuned)", but ConstraintConfiguration.enabled defaults to True, so that config has primary/foreign/unique/check constraints all enabled - the same "constraints-only" state the FALLBACK resolution path already labels correctly elsewhere in this module. A user comparing against --tuning notuning (all constraints disabled) could read "untuned" and assume parity with that baseline when it isn't one. Message-only fix; the constraints-enabled behavior itself is unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6 --------- Co-authored-by: Claude <noreply@anthropic.com>
joeharris76
pushed a commit
that referenced
this pull request
Jul 16, 2026
…un.py's size guard Merges origin/develop (PR #1170 explorer ingest, #1177 soundness tests, #1172 author-experience quickfixes, #1174/#1175) into the provenance branch and fixes the resulting module-size guardrail trip. Merge: git merge auto-resolved cleanly (no textual conflicts) - the two branches touched disjoint regions of every shared file: - benchbox/cli/tuning_resolver.py: #1172 reordered resolve_tuning()'s keyword-vs-path-existence checks (Case 3/4 swap) in a different function than my new resolve_template_reference()/promote_tuning_ provenance() additions; both compose without change. - benchbox/core/tuning/interface.py: #1174's module-level platform- compatibility-map extraction landed near the top of the file (~line 98-213); my UnifiedTuningConfiguration.get_configuration_hash() addition is far below (~line 1525), alongside the pre-existing BenchmarkTunings.get_configuration_hash() (~line 908) - both hash methods verified present and correctly scoped to their own class. benchbox/platforms/base/adapter.py: 40 lines changed by develop, none overlapping my tuning_source/tuning_source_file/tuning_config kwarg reads. - tests/unit/cli/test_tuning_resolution.py: develop added test_tuned_keyword_not_shadowed_by_local_path and test_notuning_keyword_not_shadowed_by_local_path; my TestResolveTemplateReference class sits after them - both sets present and passing. - benchbox/core/results/schema.py, models.py, builder.py, loader.py, base.py, result_capture.py, duckdb.py, result_factory.py: untouched by develop's PRs, so my provenance work there is unaffected. Fix: the merge pushed benchbox/cli/commands/run.py to 3000 lines against its 2946-line allowlist entry (tests/system/test_module_size_thresholds.py). Rather than bump the allowlist, relocated the tuning-provenance-promotion logic to benchbox/cli/tuning_resolver.py as a new top-level function, promote_tuning_provenance() (positional args, not the SimpleNamespace-typed private helper run.py had), and shortened its 4 call sites in run.py to one line each. Also deduplicated the three near-identical "tuning-related DatabaseConfig override" dict-building blocks (_dry_run_build_db_config, _run_direct, _data_or_load_build_db_config) into one shared _tuning_override_entries() helper (pure mechanical extraction - the three override dicts differed only in a couple of extra keys, kept explicit at each call site). Net effect: run.py is back to exactly 2946 lines (the existing limit, unchanged) with no functional difference - re-verified below. Also dropped a defensive `return None` in _data_or_load_build_db_config's except branch that shadowed the return-None-on-failure fallthrough pattern already used identically by its sibling _run_direct (both rely on click's ctx.exit(1) to terminate; matches existing codebase convention, saved one line). Verification: - BENCHBOX_SKIP_TEST_LOCK=1 uv run -- python -m pytest tests/unit/core/results tests/unit/core/tuning tests/unit/cli/test_tuning* -q -n 0: 1298 passed, 3 skipped. - BENCHBOX_SKIP_TEST_LOCK=1 uv run -- python -m pytest tests/unit/cli -q: 1489 passed (3 pre-existing, environment-only failures unrelated to this change - root-permission artifacts in output-directory validation tests, confirmed via git stash comparison against develop HEAD before this branch). - uv run -- python -m pytest tests/system/test_module_size_thresholds.py tests/uat/test_no_cli_surface_drift.py -q: 6 passed (no CLI-surface-drift hits on any file this branch touches; no allowlist bump needed). - make lint: clean. - E2E: `benchbox run --platform duckdb --benchmark tpch --scale 0.01 --tuning tuned --non-interactive` (via BENCHBOX_TUNING_PATH pointed at the FK-disabled template copy, per the original PR's notes) still produces platform.tuning with tuning_source=auto_discovered and a populated requested_config_hash, unchanged after the merge and refactor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QTnjb2i44JUL71kmu9sk1t
joeharris76
added a commit
that referenced
this pull request
Jul 16, 2026
…n, naive/aware datetime crash (#1179) Consolidates three late-landing chatgpt-codex-connector review findings on PRs (#1172, #1174, #1171) that merged before the sweep could reach their branches: - benchbox/core/dryrun.py: _extract_ddl_preview called benchmark.get_schema().keys() unconditionally, but public wrapper benchmarks (e.g. JoinOrder) return a DDL string from get_schema() rather than a mapping, raising AttributeError and turning a tuned dry-run's DDL preview into a silent warning. Normalizes dict/list schemas the same way _generate_external_schema_sql() already does, falling back to an empty preview for anything else. - benchbox/core/tuning/metadata.py: the ClickHouse CREATE TABLE SQL used base_sql.replace(")", ...) to append the ENGINE clause, which rewrote every closing parenthesis in the statement -- including each VARCHAR(N) column width -- producing invalid DDL. base_sql already ends with the CREATE TABLE's closing paren, so appending the clause is sufficient and touches nothing else. - tests/uat/phases/report.py: release_gate_ordering_violations compares parsed Docker uat_lifecycle.log timestamps (naive local time, written by append_lifecycle_log) against native_stage_completed_at, which is offset-aware in production (orchestrator.py's datetime.now().astimezone(), #1162). Comparing naive and aware datetimes raised TypeError, crashing make uat-gate-check whenever a Docker stage had an action=up lifecycle entry. Normalizes the naive side onto native_stage_completed_at's awareness before comparing, preserving the existing naive-to-naive behavior other callers rely on. Claude-Session: https://claude.ai/code/session_01SKw2UhgHuzYPo3y5MfarD6 Co-authored-by: Claude <noreply@anthropic.com>
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
2 participants
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.
Pull Request
Description
Implements TODO
tuning-author-experience-honesty-quickfixes-20260712(review findings R3/N1/N2/N3). Four fixes:benchmark.get_tables()which exists on no benchmark class (switched toget_schema()), and_build_table_ddl_entryreading nonexistentTuningClausesfields (switched toget_inline_clauses()). A tuned duckdb/tpch dry-run now renders 12 ORDER BY clauses (was 0).physical_rendering_idincluded per review nit).--dry-runflag-swallow — option-like values (leading-) rejected with a clear message;--dry-run --non-interactiveno longer creates a directory named--non-interactive.tuned/notuning/auto) resolve before path existence (a local file/dir namedtunedcan no longer hijack the mode); invalid-value hints now saybenchbox tuning listinstead of the self-referential--tuning list.Moves the TODO to DONE.
Type of Change
Testing
make lintclean;tyno new diagnosticstuned/directory shadow test resolves via keyword; hint text verifiedtest_dry_run_with_missing_output_dirfails as root on unmodified develop too (mkdir/invalidsucceeds as root) — unrelatedPublic Contract Check
--dry-run -foonow rejected; keyword shadowing removed) — no documented contract surface altered.Documentation
tuning-docs-env-contract-fixes-20260712.tuned/notuning/auto(no extension) is no longer loadable as an implicit template — pass an explicit path with extension.Code Quality
Artifact Hygiene
Notes
Skipped Nit (out-of-scope file, on record for the renderer-consolidation TODO):
benchbox/cli/dryrun.py:755-758still reads never-populated legacy keysdistribution_style/distribution_keyin the summary display — harmless (.get()guarded; values render in the SQL block).Part of the tuning remediation batch (#1161).
🤖 Generated with Claude Code
https://claude.ai/code/session_01QTnjb2i44JUL71kmu9sk1t
Generated by Claude Code