Split typechecking job into parts #729
Conversation
📝 WalkthroughWalkthroughThis PR refactors the CI/CD pipeline typecheck job infrastructure by splitting monolithic typecheck jobs into multi-part variants. Specifically, it replaces single typecheck jobs (for JDK 17, 21, and 25 across both bundled and latest CF configurations) with three-part decompositions (part1, part2, part3) across both Azure Pipelines and CircleCI. The timeout per job is reduced from 80 to 40 minutes. New parameterized macros are introduced to support this multi-part structure, and test scripts are updated to accept an optional part argument that selects which partition of the typecheck job to execute. Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.azure/defs.m4 (1)
102-124: Remove the unusedtypecheck_jobmacro.The
typecheck_jobmacro (lines 103–124) is dead code—no invocations exist anywhere in the codebase. Onlytypecheck_job_parts(which callstypecheck_job_part) is invoked. Additionally, this macro has inconsistent argument handling: it references$4in the job name (line 104) but the dependency references at lines 107 and 109 omit$4, unlike the corresponding lines intypecheck_job_part(lines 134–137) which correctly include it.If this macro is intentionally kept for future use, fix the dependency references to include the part argument (
$4)..circleci/config.yml (1)
1083-1090: Canary-jobs references non-existent jobtypecheck-bundled-ubuntu-jdk25.The
canary-jobsdepends ontypecheck-bundled-ubuntu-jdk25, but this job has been renamed to part-based variants (typecheck-bundled-part1-ubuntu-jdk25, etc.). This will cause the workflow to fail.🔎 Proposed fix
- canary-jobs: requires: - quick-txt-diff-ubuntu-jdk25 - nonquick-txt-diff-ubuntu-jdk25 - non-txt-diff-ubuntu-jdk25 - misc-ubuntu-jdk25 - kvasir-ubuntu-jdk25 - - typecheck-bundled-ubuntu-jdk25 + - typecheck-bundled-part1-ubuntu-jdk25 + - typecheck-bundled-part2-ubuntu-jdk25 + - typecheck-bundled-part3-ubuntu-jdk25.azure/azure-pipelines.yml (1)
24-32: Canary_jobs references non-existent jobstypecheck_latest_ubuntu_jdk25andtypecheck_bundled_ubuntu_jdk25.The
canary_jobsdepends on jobs that have been renamed to part-based variants. This will cause the pipeline to fail with "Job not found" errors.🔎 Proposed fix
- job: canary_jobs dependsOn: - quick_ubuntu_jdk25 - nonquick_ubuntu_jdk25 - nontxt_ubuntu_jdk25 - misc_ubuntu_jdk25 - kvasir_ubuntu_jdk25 - - typecheck_latest_ubuntu_jdk25 - - typecheck_bundled_ubuntu_jdk25 + - typecheck_latest_part1_ubuntu_jdk25 + - typecheck_latest_part2_ubuntu_jdk25 + - typecheck_latest_part3_ubuntu_jdk25 + - typecheck_bundled_part1_ubuntu_jdk25 + - typecheck_bundled_part2_ubuntu_jdk25 + - typecheck_bundled_part3_ubuntu_jdk25
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
.azure/azure-pipelines.yml(1 hunks).azure/defs.m4(3 hunks).azure/jobs.m4(1 hunks).circleci/config.yml(7 hunks).circleci/config.yml.m4(1 hunks).circleci/defs.m4(2 hunks).circleci/jobs.m4(1 hunks)scripts/test-typecheck-with-bundled-cf.sh(2 hunks)scripts/test-typecheck-with-latest-cf.sh(2 hunks)
🔇 Additional comments (14)
.azure/jobs.m4 (1)
31-39: LGTM!The migration from individual
typecheck_bundled_job/typecheck_latest_jobmacros to the parameterizedtypecheck_job_partsmacro is clean and consistent. The comments correctly document the JDK version constraints for the Checker Framework..azure/defs.m4 (1)
125-151: LGTM!The multi-part job structure is well-designed:
typecheck_job_partscleanly generates three sub-jobstypecheck_job_partcorrectly includes the part identifier in job names and dependencies- The 40-minute timeout per part (vs 80 for the original monolithic job) is reasonable for parallel execution
- Script invocation correctly passes the part argument
.circleci/jobs.m4 (1)
1-1: LGTM!Clean reuse of Azure job definitions in CircleCI via m4 include. This avoids duplication and ensures consistency between CI systems.
.circleci/config.yml.m4 (2)
34-34: Verify canary-jobs dependency reference.Line 34 references
typecheck-bundled-ubuntu-jdk[]canary_version, but with the new multi-part structure, job names are nowtypecheck-bundled-part1-ubuntu-jdk25,typecheck-bundled-part2-ubuntu-jdk25, etc. This reference may not match any actual job name.Should this require all three parts, or one specific part (e.g.,
typecheck-bundled-part3-ubuntu-jdk[]canary_versionto ensure all parts complete)?#!/bin/bash # Check what job names are generated in the Azure config fd -e yml -e yaml azure-pipelines --exec cat {} 2>/dev/null | grep -E 'typecheck.*ubuntu.*jdk25' | head -20 # Also check the generated CircleCI config if available fd -e yml config . -p '.circleci' --exec cat {} 2>/dev/null | grep -E 'typecheck.*ubuntu.*jdk25' | head -20
64-78: Thejob_dependences_partmacro is properly defined in.circleci/defs.m4and all usages at lines 64-78 are correct. No issues found.Likely an incorrect or invalid review comment.
scripts/test-typecheck-with-bundled-cf.sh (1)
41-45: Potential bug: duplicated "part" prefix in make target construction requires verification.The review identifies a logical concern about target name construction at line 44: if the script receives arguments like
part1,part2,part3(as documented in CI macro definitions), constructing the target astypecheck-part$1would producetypecheck-partpart1with a doubled "part" prefix.However, this bug claim cannot be verified without access to the actual Makefile to confirm:
- The exact make target names defined in
java/Makefile- The actual arguments passed to the script from
.azure/defs.m4The proposed fix should only be applied after confirming the actual target naming convention in the repository.
scripts/test-typecheck-with-latest-cf.sh (1)
35-39: LGTM!The CI detection and job count logic is correct. Using
.plume-scripts/is-ci.shis consistent with expected script locations..circleci/config.yml (3)
538-603: LGTM!The three-part typecheck-bundled jobs for JDK17 are correctly structured with consistent environment, caching, and script invocations.
670-735: LGTM!The typecheck-latest-part{1,2,3}-ubuntu-jdk21 jobs are correctly defined with appropriate script invocations.
1200-1250: LGTM!The workflow dependencies are correctly updated to use per-part job names, with proper dependency chains from JDK17/21 parts to their corresponding JDK25 parts.
.circleci/defs.m4 (2)
72-96: LGTM!The new macros are well-structured:
typecheck_jobfor full typecheck runs without partstypecheck_job_partsto generate all three part jobstypecheck_job_partfor individual part definitionsThe script invocation correctly passes the part argument (line 95).
111-123: LGTM!The
job_dependences_partmacro correctly mirrors the existingjob_dependencesmacro structure while adding the fourth argument for part handling in dependency chains..azure/azure-pipelines.yml (2)
343-393: LGTM!The three-part typecheck_bundled jobs for JDK17 are correctly structured with:
- Proper dependencies on canary_jobs and corresponding JDK25 parts
- 40-minute timeout (reduced from 80, appropriate for split jobs)
- Correct script invocations with part arguments
496-579: LGTM!The JDK25 typecheck jobs (both bundled and latest, all three parts) are correctly defined as canary jobs without dependencies, with proper 40-minute timeouts.
| if [ "$#" -ge 2 ]; then | ||
| echo "$0: expected 0 or 1 arguments." | ||
| exit 2 | ||
| fi | ||
|
|
||
| if [ "$#" -eq 0 ]; then | ||
| make -C java --jobs="$num_jobs" typecheck | ||
| else | ||
| make -C java --jobs="$num_jobs" "typecheck-part$1" | ||
| fi |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider validating the part argument value.
The script accepts any string as argument and will pass it to make as typecheck-part$1. If an invalid value like "foo" is passed, the error will come from make (target not found) rather than a clear usage message.
🔎 Proposed validation
if [ "$#" -ge 2 ]; then
echo "$0: expected 0 or 1 arguments."
exit 2
fi
+if [ "$#" -eq 1 ] && [ "$1" != "part1" ] && [ "$1" != "part2" ] && [ "$1" != "part3" ]; then
+ echo "$0: argument must be part1, part2, or part3, not '$1'."
+ exit 2
+fi
+
if [ "$#" -eq 0 ]; then
make -C java --jobs="$num_jobs" typecheck
else
make -C java --jobs="$num_jobs" "typecheck-part$1"
fi🤖 Prompt for AI Agents
In scripts/test-typecheck-with-latest-cf.sh around lines 41 to 50, the script
currently accepts any string as the part argument and forwards it to make as
"typecheck-part$1", which yields unclear make errors for invalid values;
validate the argument before calling make by checking that when an argument is
provided it matches the expected pattern (e.g. a positive integer with a regex
like '^[1-9][0-9]*$') and optionally verify it is within the known parts range
if that value is available; if validation fails, print a clear usage message
(showing valid values or format) and exit with a non-zero status instead of
invoking make.
1 participant
No description provided.