Skip to content

⚡ Bolt: Consolidated Performance Optimizations Batch - #311

Closed
dieterolson wants to merge 11 commits into
mainfrom
consolidated/bolt-optimizations-batch
Closed

⚡ Bolt: Consolidated Performance Optimizations Batch#311
dieterolson wants to merge 11 commits into
mainfrom
consolidated/bolt-optimizations-batch

Conversation

@dieterolson

Copy link
Copy Markdown
Contributor

Consolidating open performance optimization PRs into a single batch to reduce CI/CD load.

Closes #307, Closes #309, Closes #310

google-labs-jules Bot and others added 9 commits August 4, 2026 05:21
Co-authored-by: dieterolson <198168927+dieterolson@users.noreply.github.com>
Co-authored-by: dieterolson <198168927+dieterolson@users.noreply.github.com>
Update actions/checkout from v7 (which doesn't exist) to v4 across all GitHub Actions workflows to fix the CI failure.

Co-authored-by: dieterolson <198168927+dieterolson@users.noreply.github.com>
Update actions/checkout from v7 (which doesn't exist) to v4 across all GitHub Actions workflows to fix the CI failure. Also install `coverage` to resolve a missing dependency error in `pytest-cov`.

Co-authored-by: dieterolson <198168927+dieterolson@users.noreply.github.com>
Co-authored-by: dieterolson <198168927+dieterolson@users.noreply.github.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bf3017c016

ℹ️ 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".

Comment on lines +99 to +101
assert (
exercises_dir.is_dir()
), f"resolved exercises dir does not exist: {exercises_dir}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Restore Ruff-compatible formatting

When the quality-gate job reaches its formatting step, ruff format --check src scripts tests examples benchmarks exits nonzero: running that exact command reports this file, sdf_helpers.py, and four modified test files as needing reformatting. This makes CI fail for every run of this commit, so these formatting-only rewrites need to match the repository's configured Ruff formatter.

AGENTS.md reference: AGENTS.md:L44-L45

Useful? React with 👍 / 👎.

@dieterolson

Copy link
Copy Markdown
Contributor Author

Reviewed in detail. Not merging this — it fails the required quality-gate check, it would regress main in three separate ways, and after tracing what it actually delivers, only one of the three PRs it claims to close still has anything new in it.

What this PR actually delivers

I diffed the branch against its merge-base (93400b5) with line endings normalised, because the raw diff is misleading — it reports 164+/158- and 190+/187- for files whose real change is +6 and +3 lines. The complete semantic change set is:

Claim Reality
Closes #310 Already merged on 2026-08-11. 93400b5, the merge-base of this very branch, is the #310 merge commit. Nothing here contributes to it.
Closes #307 (np.where for NaN cleanup) Already on main via #308 (b904fa9). The merge-base already has clean_arr = np.where(np.isnan(arr), 0.0, arr) with its explanatory comment at objectives/__init__.py:181-183. This PR's only contribution is a duplicate of that comment at lines 184-186, restating lines 181-183 with no code change.
Closes #309 (dict lookup) Genuinely new, and correct — see below.

So the entire net value is one micro-optimization plus two .jules/bolt.md knowledge entries.

The one good change, src/drake_models/optimization/drake_trajectory_solver.py:253-255:

idx = joint_name_to_idx.get(jname)
if idx is not None and idx < n_q:
    target[idx] = angle

This is correct. joint_name_to_idx is built from enumerate, so its values are always int and never None, which makes is not None a safe sentinel test rather than a value collision. It does halve the dict lookups per joint per phase. I have no objection to this hunk.

Blockers

1. quality-gate will fail — the branch was formatted with black, not ruff. The head commit is titled "style: format python files with black/ruff", and quality-gate runs ruff format --check src scripts tests examples benchmarks (.github/workflows/ci-standard.yml:59). Verified with ruff 0.16.1:

  • merge-base 93400b5: 109 files already formatted, ruff check -> All checks passed
  • this branch bf3017c: 6 files would be reformatted

The six, first offending line each: src/drake_models/model_pack.py:99, src/drake_models/shared/utils/sdf_helpers.py:325, tests/test_drake_biomechanics_validation.py:201 (25 assert statements in that file alone), tests/unit/exercises/test_gait.py:43, tests/unit/shared/test_body_model.py:207, tests/unit/shared/test_ground_contact.py:230. black's assert (\n cond\n), msg layout and ruff-format's assert cond, (\n msg\n) layout disagree, and main is committed in the ruff form. main is 100% clean, so this PR is the sole regression source.

2. actions/checkout downgraded v7 -> v4 in six workflows. Nothing to do with performance, and it reverses infrastructure already on main:

  • .github/workflows/anti-phantom-merge.yml:28
  • .github/workflows/ci-standard.yml:42, :107, :156
  • .github/workflows/lint-workflow-files.yml:21
  • .github/workflows/local-only-runner-guard.yml:32
  • .github/workflows/model-pack-smoke.yml:35
  • .github/workflows/rust-ci.yml:44

Also unexplained: ci-standard.yml:52 adds a hard coverage==7.15.4 pin to the lint install line.

3. Two files rewritten to CRLF in an all-LF repo. Byte-level counts against the merge-base:

  • .jules/bolt.md: 26,953 B / 158 LF, 0 CRLF -> 28,106 B / 164 CRLF, 0 LF
  • src/drake_models/optimization/objectives/__init__.py: 7,685 B / 187 LF, 0 CRLF -> 8,103 B / 190 CRLF, 0 LF

This is what inflates the diff and it would land a mixed-EOL tree.

On correctness specifically

I read every hunk for the accumulator-initialization failure mode — an optimization that deletes an intermediate array and silently takes its initializer with it, leaving an UnboundLocalError on a path that only fires at runtime. Not present here, and it cannot be: the semantic diff contains no removed array allocations, no inlined exponentiation, and no hoisted loop invariants. Despite the title, nothing in this PR touches numeric code at all. There are no math hunks to get wrong.

Suggested path forward

The cheapest fix is not to repair this branch — three of its four changes are damage and one is a duplicate comment. Re-cut it as a small PR carrying only the #309 dict.get() hunk plus the two new .jules/bolt.md entries, written with LF endings and formatted with ruff format (not black). Then close #307 and #309 as superseded, noting that #307's substance already landed via #308.

Leaving this open rather than closing it so the #309 work isn't lost, but it should not be merged in its current shape.

@dieterolson
dieterolson disabled auto-merge August 14, 2026 04:02
@dieterolson

Copy link
Copy Markdown
Contributor Author

Auto-merge (SQUASH) was armed on this PR. Disarmed — this branch was reviewed
in the 2026-08-13 consolidation pass and rejected on content, so it must not
merge itself the moment someone makes the gate go green:

  • actions/checkout is downgraded v7 -> v4 in eight places
    (anti-phantom-merge.yml:28, ci-standard.yml:42/:107/:156,
    lint-workflow-files.yml:21, local-only-runner-guard.yml:32,
    model-pack-smoke.yml:35, rust-ci.yml:44), plus an unexplained
    coverage==7.15.4 pin at ci-standard.yml:52. v7.0.1 is the current upstream
    release and main is on v7.
  • .jules/bolt.md and src/drake_models/optimization/objectives/__init__.py are
    rewritten entirely to CRLF (164/0 and 190/0 CRLF/LF) in an all-LF repo,
    which is what inflates the diff to 164+/158- for a +6-line change.
  • Formatted with black, not ruff: at ruff 0.16.1 main reports
    "109 files already formatted" while this head has 6 files that would be
    reformatted
    (model_pack.py:99, sdf_helpers.py:325,
    test_drake_biomechanics_validation.py:201, test_gait.py:43,
    test_body_model.py:207, test_ground_contact.py:230). quality-gate runs
    ruff format --check, so this fails as-is.
  • Duplicate comment block at objectives/__init__.py:184-186.
  • Two of the three claimed PRs are already done: ⚡ Bolt: [performance improvement] Replace np.tile with np.repeat for constraint bounds #310 is already merged
    (93400b5, which is this branch's own merge-base) and ⚡ Bolt: Replace array copy+mask with np.where for faster NaN cleanup #307's substance is on
    main via ⚡ Bolt: [performance improvement] Vectorize NaN replacement in clean array caching #308 (b904fa9).

Despite the "Performance Optimizations" title, nothing here touches numeric code —
there are no removed array allocations, no inlined exponentiation and no hoisted
invariants in the EOL-normalized diff.

The only genuinely new content is #309's one-liner at
drake_trajectory_solver.py:253-255, and that hunk is correct. Re-cut it as a
small PR off current main (LF endings, ruff-formatted, no actions/checkout
changes) and close this one.

@dieterolson
dieterolson enabled auto-merge (squash) August 14, 2026 04:31
@dieterolson
dieterolson disabled auto-merge August 14, 2026 04:55
@dieterolson

Copy link
Copy Markdown
Contributor Author

Closing — content rejected, and disarming auto-merge does not hold

Per the consolidation lead's ruling: disarming auto-merge is not durable in this org
(a scheduled workflow re-armed Gasification_Model #4709 at 2026-08-14T04:02:09Z
after it had been disarmed and verified null, attributed to dieterolson but
acting with the owner token; the same happened to Tools #4409/#4410). Closing is the
only state that sticks, so a PR that must not merge has to be closed rather than
merely disarmed.

This PR was rejected on content in the 2026-08-13 pass — full detail in the review
comment above. Summary:

  • actions/checkout downgraded v7 → v4 in eight places
    (anti-phantom-merge.yml:28, ci-standard.yml:42/:107/:156,
    lint-workflow-files.yml:21, local-only-runner-guard.yml:32,
    model-pack-smoke.yml:35, rust-ci.yml:44), plus an unexplained
    coverage==7.15.4 pin at ci-standard.yml:52. v7.0.1 is current upstream and
    main is on v7.
  • Two files rewritten entirely to CRLF (.jules/bolt.md and
    src/drake_models/optimization/objectives/__init__.py, 164/0 and 190/0 CRLF/LF)
    in an all-LF repo — which is what inflates the diff to 164+/158− for a +6-line
    change.
  • Formatted with black, not ruff. At ruff 0.16.1 main reports "109 files
    already formatted" while this head has 6 files that would be reformatted
    (model_pack.py:99, sdf_helpers.py:325,
    test_drake_biomechanics_validation.py:201, test_gait.py:43,
    test_body_model.py:207, test_ground_contact.py:230). quality-gate runs
    ruff format --check, so this cannot go green as-is.
  • Duplicate comment block at objectives/__init__.py:184-186.
  • Two of the three claimed PRs are already done: ⚡ Bolt: [performance improvement] Replace np.tile with np.repeat for constraint bounds #310 is already merged
    (93400b5, which is this branch's own merge-base) and ⚡ Bolt: Replace array copy+mask with np.where for faster NaN cleanup #307's substance is on
    main via ⚡ Bolt: [performance improvement] Vectorize NaN replacement in clean array caching #308 (b904fa9).

Despite the "Performance Optimizations" title, nothing here touches numeric code —
no removed array allocations, no inlined exponentiation, no hoisted invariants in the
EOL-normalized diff.

For the record on the gating: this repo returns 404 for
branches/main/protection, but it is not unprotected — org ruleset 16717249
requires quality-gate, ruleset 14239695 requires quality-gate and
tests (3.11), and ruleset 15613814 requires Reject hosted runner routing.

The branch consolidated/bolt-optimizations-batch is not deleted — closing a PR
preserves the ref, so nothing is lost.

The one genuinely new and correct piece of work here is #309's one-liner at
drake_trajectory_solver.py:253-255. Re-cut that as a small PR off current main
(LF endings, ruff-formatted, no actions/checkout changes) and it should sail
through.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant