ci: reduce test overhead and unshard to conserve runners and reduce flakes - #686
Conversation
Castiron custom code✅ No new custom-code files detected. 50 mixed files remain; 0 existing customizations changed. Compared 50 existing customizations unchanged
10 more in the full report. A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 34255805262 --repo openai/openai-ruby \
--name castiron-custom-code-34255805262-1 --dir /tmp/castiron-custom-code-34255805262-1
git apply --stat /tmp/castiron-custom-code-34255805262-1/custom-code.patch
cat /tmp/castiron-custom-code-34255805262-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 31bbd60de2775f90f46dcaddda8089d5e322bc5b c03d0ed004366b0e283581a74bacf9c7a75b6dbc
python3 scripts/castiron/custom_code_report.py report \
--base 31bbd60de2775f90f46dcaddda8089d5e322bc5b \
--head c03d0ed004366b0e283581a74bacf9c7a75b6dbc --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-c03d0ed00436
cat /tmp/castiron-custom-code-c03d0ed00436/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
9db65bc to
95b780e
Compare
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Reviewed the complete exact-head 23-file diff against the test-efficiency outcome and the stated non-goals. The consolidated RBS harness preserves the ten separate consumer files and exact negative diagnostics, the pairwise RBI merge retains within-pair, cross-pair, and odd-tail conflict coverage, and the initialized-Ruby PID publication closes the lost-TERM fixture race with a bounded launcher wait. git diff --check passes, no runtime/signature/dependency/generated-surface changes are present, and all completed exact-head lint/type/package/RBS/Bedrock/CodeQL/Castiron checks are green. One blocking scope/CI regression remains: this head undoes the just-merged three-way Ruby test matrix even though the PR description says job selection is unchanged and cites sharded validation from an older head. Restore the three-way matrix, or explicitly re-scope and revalidate the unsharding decision on the current head.
![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: 95b780e12d
ℹ️ 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".
apcha-oai
changed the title
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed all 23 changed files against main/base 31bbd60d, including the surrounding RBS/RBI and launcher contracts. One substantive issue remains: the balanced RBI merge is not equivalent to the original ordered merge and can silently lose a conflict (reproduction inline). Collecting every level's conflict metadata does not fix conflicts erased before that level is merged.
The RBS consolidation otherwise removes duplicated setup cleanly: all ten consumer sources remain separate, and an independent mutation produced exactly one missing and one unexpected diagnostic. Both original Client superclass regressions are still detected. No file crosses 1,000 lines, and no SDK runtime/signature/dependency/generated-surface changes are present.
Fresh local Ruby 4.0.6 validation: 23 focused tests / 115 assertions passed; the interrupt test passed (1 test / 3 assertions). The broader launcher run had one readiness-timeout assertion failure, so I am not claiming a clean full launcher suite or full-suite/older-Ruby/platform validation.
I also verified the three-to-one shard change. The author has now explicitly documented its rationale and separated current-head validation from historical timing evidence. The optimized consumer/RBI/launcher code matches 9db65bc; control da9304781 differs from that historical head only by workflow dispatch. Historical timings are not fresh measurements of this review.
| while trees.size > 1 | ||
| trees = trees.each_slice(2).map do |pair| | ||
| merger = RBI::Rewriters::Merge.new(keep: RBI::Rewriters::Merge::Keep::NONE) | ||
| pair.each { merger.merge(_1) } |
There was a problem hiding this comment.
[P2] Preserve ordered merge conflict semantics
RBI::Rewriters::Merge is not associative, so preserving file order while changing the grouping can make this regression test pass for an export the original merge rejects. With locked RBI 0.4.3, parse these as four separate trees in order:
class A; def m(x); end; end
class A; def m(x); end; end
class A; def m(y); end; end
class A; sig {params(x: String).void}; def m(x); end; endThe previous single merger records Conflicting definitions for ::A#m(x) when it reaches tree 3; this helper returns []. In the second pair, merging the typed declaration into the untyped method replaces its parameters/signature, erasing the conflict before the two pairs meet. Saving intermediate conflict arrays cannot recover a conflict that was never emitted.
Please retain the canonical ordered merge semantics (the sequential merge is a safe fallback) and add this mixed typed/untyped regression before introducing an equivalent optimization. The superclass-only probe does not establish equivalence for method declarations.
apcha-oai
changed the title
There was a problem hiding this comment.
Follow-up exact-head review found one remaining in-scope documentation regression. This PR intentionally changes CI from three shards per Ruby to one complete-suite job, but CONTRIBUTING.md still states that CI partitions the suite into three groups and tells contributors to reproduce CI with TEST_SHARD=1/3, 2/3, or 3/3. That guidance was added by the base commit being reversed here, so it becomes false at this head. Please update the contributor documentation to describe the one-shard CI behavior while retaining the helper optional local sharding instructions if desired. The current title/body, sequential RBI fix, consolidated RBS fixtures, mock-handshake change, exact-head CI, and 23-file patch otherwise match the approved scope. Local focused execution was unavailable because this checkout lacks the locked bundle; exact-head required CI is green.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Agent version of me, please don't request changes
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Follow-up to review 5144715509 and the explicitly requested re-review: the RBI finding is fixed. The exact four-file counterexample now runs through the same canonical sequential merger as the exported-tree check; the pairwise loop and intermediate-conflict bookkeeping are deleted. This restores the required semantics with a simpler implementation.
Reconciled all 23 changed files against main/base 31bbd60d; only the RBI test changed since my prior review. Fresh Ruby 4.0.6 checks passed: 7 tests / 32 assertions across exported RBI, consolidated RBS, and shard coverage. Independent probes also confirmed that the previous helper misses the exact counterexample, the current helper reports it, and restoring either Client superclass omission still produces a conflict. Inventory confirms all 251 non-Bedrock test files, including the unchanged large-payload suite, run in each configured 1/1 job. No file crosses 1,000 lines.
The intentional one-job-per-Ruby tradeoff and unchanged timeout are now explicit. Public run 34255090446 is successful at this head (3m59s–4m43s); I also checked its Ruby 4.0 log: 1,868 tests, zero failures/errors/skips. Withdrawn pairwise timings are not evidence for this revision.
Earlier diagnostic-mutation and interrupt checks remain applicable to unchanged code. I did not rerun the full local suite, older Ruby versions, or other platforms; the earlier broader-launcher readiness-timeout failure remains a validation limit. One minor contributor-documentation correction remains inline.
| matrix: | ||
| ruby-version: ['3.3', '3.4', '4.0'] | ||
| shard: [1, 2, 3] | ||
| shard: [1] |
There was a problem hiding this comment.
[P3] Align contributor instructions with the intentional one-shard workflow
The one-shard tradeoff is now explicit and the configured inventory is complete, but CONTRIBUTING.md:152–156 still says CI partitions each Ruby suite into three groups and presents TEST_SHARD=1/3, 2/3, and 3/3 as reproducing CI. Please describe the complete-suite CI job there and label those commands as optional local sharding. The helper can remain available without documenting it as the current CI configuration.
3 participants
Ruby CI repeatedly copied and loaded the full RBS signature tree for ten tiny consumer checks, and a mock-process startup race could add a 60-second wait. This change batches the compatibility fixtures, fixes the mock handshake, and returns the Ruby matrix to one shard per version for simplicity and fewer opportunities for flakes from repeated job setup.
Why return to one shard
Unsharding is intentional. Three-way sharding brought elapsed CI time back toward baseline, but multiplied the Ruby matrix from three jobs to nine. That consumes more runners and creates more opportunities for flakes through additional dependency installation, mock startup, and cleanup. We prefer to remove redundant test work and keep one complete-suite job per Ruby version; sharding should be a last resort if further test improvements cannot provide acceptable feedback time.
This is a deliberate tradeoff: individual unsharded jobs take longer than shards, while CI uses fewer runners and has fewer independent setup/cleanup paths. The current sequential-RBI implementation completes public jobs in 3m59s–4m43s and internal jobs in 5m11s–5m35s. The original under-five-minute target is not met on the internal runners, and we intentionally retain one shard rather than restore nine jobs to meet it.
Changes
--with-expectations. Runtime and declaration assertions remain in their original suites.Rebased onto main
31bbd60de2775f90f46dcaddda8089d5e322bc5b. The upstream 15-minute timeout from #683 is unchanged; this PR changes the three-way sharding added in #684. No SDK runtime, signature, dependency, or generated-surface changes. Large-payload tests retain their original sizes and assertions.Root cause and remaining cost
Internal Ruby 3.4 test execution grew from 2.9 seconds on July 15 (460 tests), to 31.1 seconds on August 14 (723 tests), to 498.4 seconds on September 8 (1,872 tests). Expanded coverage explains some growth; the measured overhead above identifies specific additional costs.
Validation
c03d0ed004366b0e283581a74bacf9c7a75b6dbcin both repositories. Each complete-suite job ran 1,868 tests with no failures, errors, or skips. Earlier 2m39s–4m04s timings used the withdrawn pairwise optimization and do not describe this revision.Current hosted validation
Whole-job times include setup and cleanup; every row uses one shard.
Runs: internal, public.
Measurement sources
21b8a70d48e2623e32aa01f95503adcf3fa2ba75: internal, public. Whole Ruby jobs were 8m38s–9m26s internally and 6m49s–7m12s publicly.SDK-team review is requested. Measurements use each repository's normal runner selection and are reported separately because observed runtimes differ.
Linked counterpart: https://github.com/openai/openai-ruby-internal/pull/97