[Quality-547] add bert_tiny_v2 model and update input classifier#10756
[Quality-547] add bert_tiny_v2 model and update input classifier#10756evelyn-with-warp wants to merge 9 commits into
Conversation
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR adds a v2 ONNX input classifier model, switches classifier model files under crates/input_classifier/models/** to Git LFS, updates the v2 feature gate to load the new model, and adds local bootstrap documentation for LFS setup.
Concerns
- CI and release workflows still check out the repository without LFS enabled, so cargo/bundle jobs that do not run
./script/bootstrapcan embed Git LFS pointer stubs instead of the classifier binaries and silently ship the heuristic fallback.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| *.pdb filter=lfs diff=lfs merge=lfs -text | ||
| input_classifier/models/* filter=lfs diff=lfs merge=lfs -text | ||
| input_classifier/models/**/*tokenizer.json linguist-generated=true | ||
| crates/input_classifier/models/** filter=lfs diff=lfs merge=lfs -text |
There was a problem hiding this comment.
actions/checkout without lfs: true and prepare_environment does not run git lfs pull, so bundle jobs can embed pointer stubs and ship the heuristic fallback. Update the shared workflow checkout path with lfs: true or run git lfs pull before cargo/bundle commands.
There was a problem hiding this comment.
see the mitigation solution in PR description; for CI, we have updated to fetch from LFS as well (as seen in Tests section in PR description)
|
The change we see in crates/input_classifier/models/onnx/bert_tiny_v1.onnx in github refers to updating raw blob to lfs pointer |
actions/checkout does not fetch LFS content by default, so the crates/input_classifier/models/** ONNX models (newly LFS-tracked) and the pre-existing *.pdb debug symbols would otherwise appear as ~133-byte pointer stubs on CI runners. rust-embed bakes the models into the binary at compile time, so without this step the bundled app embeds the pointer text and silently falls back to the heuristic classifier at runtime. Adding a 'git lfs pull' step to the shared prepare_environment composite action covers every workflow that goes through it (ci.yml + create_release.yml build/test/bundle jobs). Co-Authored-By: Oz <oz-agent@warp.dev>
84580d2 to
3bf9211
Compare
| input_classifier/models/* filter=lfs diff=lfs merge=lfs -text | ||
| input_classifier/models/**/*tokenizer.json linguist-generated=true | ||
| crates/input_classifier/models/** filter=lfs diff=lfs merge=lfs -text | ||
| crates/input_classifier/models/**/*tokenizer.json -filter -diff -merge text linguist-generated=true |
There was a problem hiding this comment.
this is probably the reason why previous onnx model was checked in as raw blob not lfs: the paths did not include crates/ so those lines matched nothing
There was a problem hiding this comment.
i'm pretty sure it originally did use LFS; this probably broke when we moved the crate from ./input_classifier to ./crates/input_classifier. (i would be surprised if i failed to set this up properly when i originally did it.)
There was a problem hiding this comment.
@vorporeal does that mean git lfs was already enabled in CI?
CI didn't fail so far
pls lmk if other tests could cross-verify
There was a problem hiding this comment.
anyway , i didn't see lfs:true at every checkout, so probably worth adding/re-adding it
There was a problem hiding this comment.
i believe we never needed lfs in CI because we weren't using the classifier in unit or integration tests (because it wasn't a default feature)
Stacks on the previous `prepare_environment` LFS pull (Option A) so we get an explicit, per-call-site audit trail in the workflow files and GitHub-rendered `with: lfs: true` block in the CI logs. The redundant `git lfs pull` afterwards in prepare_environment is a no-op when checkout has already materialized the LFS objects. Touches every `actions/checkout` invocation in: - .github/workflows/ci.yml (8 sites) - .github/workflows/create_release.yml (12 sites) Merged with existing `with:` blocks (`ref:` on release_linux_arm and `fetch-depth:` on generate_changelogs) rather than duplicating them. Co-Authored-By: Oz <oz-agent@warp.dev>
Adds a follow-up Verify LFS payloads step to prepare_environment so that every CI job's log contains an explicit per-file listing of the materialized LFS payloads, e.g.: 1fd8afc92b * crates/input_classifier/models/onnx/bert_tiny_v2.onnx (17 MB) 05b2aaba0e * crates/input_classifier/models/onnx/bert_tiny_v1.onnx (17 MB) Makes it trivial to confirm at a glance that the ONNX classifier binaries reached the runner as actual blobs rather than 133-byte pointer stubs. The leading asterisk means the data is present in the working tree (not just a pointer). Co-Authored-By: Oz <oz-agent@warp.dev>
This reverts commit c0e05a7. We already verified LFS payloads load correctly via the prepare_environment-only path (commits 3bf9211 and 596a622), so the per-checkout `lfs: true` opt-ins are redundant. Drop them to keep the diff to .github/workflows/* minimal and rely on `prepare_environment` to do the single `git lfs pull`. Co-Authored-By: Oz <oz-agent@warp.dev>
|
|
||
| ### Platform Setup | ||
| - `./script/bootstrap` - Platform-specific setup (calls platform-specific bootstrap scripts) | ||
| - `./script/bootstrap` - Platform-specific setup (calls platform-specific bootstrap scripts). Also verifies `git-lfs` is installed and runs `git lfs install --local && git lfs pull` so model binaries under `crates/input_classifier/models/**` are materialized in the working tree. |
There was a problem hiding this comment.
this is not a useful update to our top-level public documentation. agents like to add extra context here - in the extreme, this lists every single thing that the bootstrap script does, which does not provide value to the end user reading our readme.
There was a problem hiding this comment.
removed in d9608a3
the initiative was giving agent some hint when there's lfs related failure, but it's a fair point that agent might able to get this from session context as well
| - `./script/install_cargo_build_deps` - Install Cargo build dependencies | ||
| - `./script/install_cargo_test_deps` - Install Cargo test dependencies | ||
|
|
||
| #### Git LFS |
There was a problem hiding this comment.
i also don't think we need this here - the bootstrap script will print out a notice with this exact same information, right?
| #[derive(Clone, Copy, RustEmbed)] | ||
| #[folder = "models/onnx"] | ||
| struct Models; |
There was a problem hiding this comment.
we should be changing this so that we're only bundling one model into the binary instead of both.
you probably want to use #[cfg_attr(feature = "...", include = "...")], to switch from unconditionally including everything in the models folder to using an allowlist
| # Print a per-file listing of the LFS payloads so the CI log shows that | ||
| # the ONNX/fasttext model binaries (and Windows .pdb files) are real | ||
| # binaries rather than 133-byte pointer stubs. The `*` marker before | ||
| # each path means "data is present in the working tree". | ||
| - name: Verify LFS payloads | ||
| shell: bash | ||
| run: | | ||
| cd "${{ steps.repo-root.outputs.path }}" | ||
| git lfs ls-files --size |
There was a problem hiding this comment.
now that we've verified that things are working properly, we can remove this, yeah? if you want to keep it, we should at least condition it on the action being executed in debug mode.
Removes the extra detail appended to the `./script/bootstrap` entry and the dedicated "#### Git LFS" subsection. Per review feedback, this level of context belongs in the bootstrap script's own output rather than in the top-level WARP.md documentation. Co-Authored-By: Oz <oz-agent@warp.dev>
Per PR review feedback, the `git lfs ls-files --size` step is only useful for on-demand auditing of LFS payload integrity. Now that we've verified things work end-to-end, gate the step on `runner.debug == '1'` so it only runs when the workflow is re-run with debug logging (or ACTIONS_RUNNER_DEBUG=true), keeping normal CI logs clean. Co-Authored-By: Oz <oz-agent@warp.dev>
12472c0 to
617cceb
Compare
Description
Add retrained nld classifier to dogfood;
Meanwhile changing model file storage from raw blobs to lfs for optimizing git efficiency
It could resolve the misfires reported in quality-547; For quality 543 on file paths misfires, some are still pending to resolve by improving heuristics.
About switching to git lfs to store model binary files
git-lfsinstalledgit clone(and subsequentgit pull) automatically invokes the smudge filter, which fetches the real ONNX blobs from LFS in place of the pointer files.rust-embedembeds the actual model bytes at build time.git-lfsinstalledrust-embedembeds the pointer file at build time; runtime tries to load it as ONNX.candle::InferenceRunner::newfails and the classifier silently falls back toHeuristicClassifier. App still builds and runs — no compile-time break, but a behavioral regression.script/bootstrapnow refuses to proceed ifgit-lfsis missing and runsgit lfs install --local && git lfs pullwhen present;WARP.mddocuments it.git-lfsinstalled but never enabled in the repogit lfs install --localhasn't been run, so the smudge filter is inactive.git lfs install && git lfs pullonce.git-lfsWARP.mdnote + the runtimelog::warn!("Failed to load onnx classifier (v2): ...")makes the failure greppable in logs.git-lfsinstalled and enabledgit pullinvokes the smudge filter automatically, fetching the v2 blob from LFS to replace the pointer.rust-embedpicks up the real bytes on the next build.git-lfslocally to render the binaries correctly..gitattributesdoesn't yet have the LFS rule.TL;DR — Risk Mitigation Summary
1. Fresh clones (hard enforcement at bootstrap)
The actual gate lives in
script/bootstrap, which now (a) refuses to proceed ifgit-lfsis not installed, and (b) runsgit lfs install --local && git lfs pullautomatically when it is. TheWARP.mdnote complements this by explaining the requirement and providing recovery commands for users who bypass bootstrap.2. Existing clones (tiered recovery)
candle::InferenceRunner::newpanics or the agent reports a missing model), the agent's remediation is to rungit lfs install --local && git lfs pullonce and retry.HeuristicClassifier, we accept it as a non-blocking degradation. Thelog::warn!("Failed to load onnx classifier (v2): ...")line makes the condition greppable in logs, and the next bootstrap run will self-heal.3. CI (requires explicit opt-in to LFS)
Update CI
with: lfs: trueto any job that compiles the app, runs classifier tests, or bundles release artifacts. Please see 3bf9211Linked Issue
ready-to-specorready-to-implement.Testing
./script/runFor the CI changes on LFS, we see below logs in CI logs to verify CI does fetch raw binary model files
macos CI
Linux CI
Windows CI
Screenshots / Videos
https://www.loom.com/share/94f99078f8614b2bbc05eab7f8d1a7d1
in this video we tested on the misfires listed in quality-547
Video for dogfood feature
https://www.loom.com/share/196ffbc863aa4b39894f4e821ab54d85
Agent Mode