-
Notifications
You must be signed in to change notification settings - Fork 209
ci: Faster typecheck
and test-generators
#2161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: canary
Are you sure you want to change the base?
Conversation
@cg-jl is attempting to deploy a commit to the Gloo Team on Vercel. A member of the Team first needs to authorize it. |
6cff07c
to
03f7ffe
Compare
whoops, I reset author in the wrong commits. I'll have to pull them back. |
d84eef3
to
7f67408
Compare
typecheck
and test-generators
typecheck
and test-generators
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed everything up to 7f67408 in 2 minutes and 57 seconds. Click for details.
- Reviewed
261
lines of code in16
files - Skipped
0
files when reviewing. - Skipped posting
7
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/primary.yml:123
- Draft comment:
Disabling goimports (install-goimports: false) speeds up CI; ensure formatting is handled elsewhere if needed. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. .github/workflows/primary.yml:165
- Draft comment:
Changing Rust setup to disable WASM and cross-compilation (enable-wasm: false, enable-cross: false) with cache-on-failure is deliberate; confirm this won’t affect any dependent WASM tasks. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
3. .github/workflows/primary.yml:170
- Draft comment:
Removal of the explicit error-handling (the fallback echo and exit) in the Test Generators step may hide generation errors; consider if this change is intentional. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The command will still fail if pnpm generate fails - bash/shell scripts exit with the last command's exit code by default. The removed error handling just added a custom error message. The next step "Ensure No Changes" still has error handling to catch any issues. The change seems intentional and not problematic. I could be wrong about how GitHub Actions handles command failures. Maybe there's a reason they had the explicit error handling before. GitHub Actions will mark a step as failed if any command in it exits with non-zero status. The explicit error handling was just for a custom message and wasn't actually needed for error detection. The comment should be deleted. The removal of explicit error handling is fine since command failures will still be caught, and the next step still has error handling.
4. engine/baml-schema-wasm/web/package.json:9
- Draft comment:
The build script now uses '--dev' mode (pnpm run pack --dev), but note the comment warns against it due to bundle size; verify that this change is as intended. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify their intention, which is against the rules. It does not provide a specific suggestion or ask for a specific test to be written. Therefore, it should be removed.
5. engine/cli/package.json:15
- Draft comment:
Specifying '--bin baml-cli' in build and release scripts is a clear improvement for targeting the correct binary. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
6. turbo.json:61
- Draft comment:
Removal of 'dependsOn' in the root 'generate' task eliminates pre-build dependencies; ensure that generation no longer relies on artifacts from CLI, Python, Ruby, or Go builds. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the author to ensure that the generation process no longer relies on artifacts from other builds. This is a request for confirmation and verification, which violates the rules. The comment does not provide a specific suggestion or point out a clear issue with the code.
7. package.json:19
- Draft comment:
Removing the '--concurrency=1' flag from the generate script may allow concurrent runs; verify thread safety of generation to avoid potential race conditions. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a speculative comment asking to "verify" something. It's not pointing to a specific issue but rather raising a hypothetical concern. Without seeing the actual generate scripts across the workspace, we can't know if there are real thread safety issues. The comment is asking the author to double-check something rather than pointing out a concrete problem. Maybe there are known thread safety issues in the generate scripts that make this a valid concern? The previous maintainer might have added the concurrency limit for a specific reason. Even if there were historical reasons, the comment doesn't provide evidence of actual issues. It's speculative and asks for verification rather than pointing out a specific problem that needs fixing. Delete the comment because it violates the rules against speculative comments and asking authors to verify/double-check things. Without concrete evidence of thread safety issues, this is not actionable.
Workflow ID: wflow_BVZ5AR2jEtOhHdq9
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
The need to run sequential may have come from undeclared dependencies between processes. Once those dependencies have been set in respective `turbo.json` files, it should run fine with concurrency. integ-test: Merged all `generate` scripts into one `generate` script in `@baml/integ-tests`. Since `baml-cli generate` generates all available languages, there is no point to have a script per API, which would cause filesystem races when cleaning up. Notes: * A clean run of `@baml/integ-tests-typescript-esm`, builds everything and takes ~17 minutes, which is the same time it takes with `--concurrency=1`. So this does not seem to be a direct fix of the timing issue.
This will remove the need to build the full workspace, opting for building only what's necessary. Will not be appreciated in uncached builds since building all of the dependencies will take most of the time in that case. Co-Authored-By: Chris Watts <[email protected]>
It was built once with "Build Node" and another time in "Test Generators".
This already runs generators for all SDKs. This removes an entire Rust build of @baml/cli. Co-Authored-By: aaronvg <[email protected]>
* Removed Go and Python setups since we're only going to run rust & typescript. * "Setup Rust" has been configured to only use bare rust, without any extras like WASM or cross-compiling.
Yeah, it's building but it doesn't look like it's generating. Maybe I need to run |
you could just copy what the previous yaml had, where you manually set the worknig directory to integ-tests/typescript and run the generate command there. It seems that maybe the generator doesn't complain if there's no baml_src. The best bet is to test locally. |
What about the errors that cannot find |
Yeah ignore those .native errord |
Otherwise `turbo` wouldn't run the generators.
If this works: FIXME: needs merging w/ commits that did *not* work out.
Brings
primary.yml
CI down to 5 minutes instead of the previously 20 minutes.Improvement mainly driven by @aaronvg's idea of ditching most of the generator processes in favor of only calling
generate
once with the TypeScript SDK, which will generate SDKs for all the supported languages in one call.Improvements remain available:
rust-unit
andpython-integration
.Important
Optimized CI by reducing generator calls, updated package scripts for efficiency, and improved integration test setup.
primary.yml
CI runtime from 20 to 5 minutes by callinggenerate
once for all SDKs.enable-wasm
andenable-cross
insetup-rust
fortest-generators
.build
,release
, andtypecheck
scripts inengine/baml-rpc/package.json
to specify--package baml-rpc
.build
anddev.skip
scripts inengine/baml-schema-wasm/web/package.json
to usepack --dev
.build
andrelease
scripts inengine/cli/package.json
to specify--bin baml-cli
.generate
scripts from severalinteg-tests/*/package.json
files.integ-tests/package.json
andinteg-tests/turbo.json
for integration test setup.This description was created by
for 7f67408. You can customize this summary. It will automatically update as commits are pushed.