Clean up nld flags#10671
Conversation
Remove the NLD-related feature flags now that the ONNX/BertTiny classifier is the production default: - Drop FeatureFlag::NLDClassifierModelEnabled and FeatureFlag::NldImprovements (including the DOGFOOD_FLAGS entry and the cfg-gated wiring in app/src/lib.rs). - Drop the nld_fasttext_model, nld_onnx_model, and nld_improvements cargo features. Enable input_classifier/onnx_candle directly on the app dependency so the BertTiny model still compiles in. - Simplify InputClassifierModel::new to always try OnnxClassifier::new(BertTiny) with HeuristicClassifier as the only fallback; remove the fasttext branch and the is_nld_classifier_enabled helper that read the EnableNLDClassifierModel user preference. - Inline the new two-option UDI segmented-control rendering in universal_developer_input.rs; the legacy build_renderable_option_config body and the duplicate build_new_renderable_option_config function are gone. - InputBufferSubmitted telemetry now fires unconditionally; the corresponding EnablementState flips to Always. - Collapse the input_mode_prefix_tests macro from 8 cases (nld_improvements x udi x InputType) to 4 (udi x InputType). - Drop the stale TODO comment in input_model.rs that referenced NldImprovements. - Remove ,nld_improvements from the macOS, Linux, and Windows bundle scripts on every channel/artifact branch. Co-Authored-By: Oz <oz-agent@warp.dev>
|
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 removes the NLD-related feature flags and cargo feature plumbing, makes the ONNX/BertTiny classifier the app default with the heuristic classifier as fallback, updates UDI rendering to the promoted two-option control, and sends InputBufferSubmitted telemetry unconditionally.
Concerns
- No blocking correctness or security concerns found in the annotated diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
The fasttext classifier was never enabled in any shipping bundle and is fully superseded by the ONNX BertTiny classifier hard-coded in `InputClassifierModel`. Drop the module, its embedded model, the `fasttext` cargo feature and its optional deps, and the fasttext branches from the `evaluate` binary. Co-Authored-By: Oz <oz-agent@warp.dev>
|
|
||
| // If we have history entries (i.e., a live session), check for | ||
| // close matches to short-circuit as shell input. | ||
| // TODO(vorporeal): decide if we still want to do this with NldImprovements. |
There was a problem hiding this comment.
Are we tracking this somewhere? Linear?
There was a problem hiding this comment.
This should be related from internal report: https://linear.app/warpdotdev/issue/QUALITY-577/improve-nld-on-dependency-for-shell-command-history
side note: looks like we are doing it today with nld improvement, but we might hope to do a better job in short-circuit with user command history cc @vorporeal
| { | ||
| // If we're submitting an AI query, we want to send telemetry for the input type. | ||
| if FeatureFlag::NldImprovements.is_enabled() { | ||
| { |
There was a problem hiding this comment.
nit: remove unnecessary scoping
| if [[ "$ARTIFACT" == "cli" ]]; then | ||
| FEATURES="$FEATURES,standalone" | ||
| elif [[ "$ARTIFACT" == "app" ]]; then | ||
| FEATURES="$FEATURES,gui,nld_improvements" |
There was a problem hiding this comment.
Per https://warpdev.slack.com/archives/C08QY6ZBK2A/p1778531062702359?thread_ts=1778529278.316949&cid=C08QY6ZBK2A, how are we ensuring that CLI builds don't bundle the model?
There was a problem hiding this comment.
CLI build will bundle the model, as a price to pay for a cleaner state (with as minimum feature flags as possible);
Here's the change 592c7b6 I was thinking of for long term: gate all app ml models together and let CLI builds ignore them; The benefit is we could edit the ml_model gate freely without changing bundle scripts, and meanwhile protected cli/oz builds from them
There was a problem hiding this comment.
it looks like we are replacing nld_improvment flag with bundled_all_ml_models flag, hopefully this will have long term benefit for readability and maintenance.
There was a problem hiding this comment.
CLI build will bundle the model, as a price to pay for a cleaner state
i don't think we're ok with paying this price. 100% of the time we would take slightly more complex code in the bundling side of things over bloating the release size by 17MB with no functional benefit.
There was a problem hiding this comment.
@vorporeal i agree for this. So current solution is we use bundled_app_ml_model flag, and the thought here is going forward this flag could handle all ml models (not only for nld work) that cli/oz should be excluded from
| lsp-types = "0.97.0" | ||
| indexmap = { version = "2.0.2", features = ["serde"] } | ||
| input_classifier = { workspace = true, features = ["onnx_candle"] } | ||
| input_classifier.workspace = true |
There was a problem hiding this comment.
features will be captured in ml_model_input_classifier_onnx
…ml_models Co-Authored-By: Oz <oz-agent@warp.dev>
592c7b6 to
5fc917a
Compare
| "nld_onnx_classifier", | ||
| ] | ||
| nld_classifier_v2 = [ | ||
| "nld_classifier_v1", |
There was a problem hiding this comment.
cc @szgupta this will be modified in the following PR when we add the new classifier
| fn tokenizer_path(&self) -> &'static str { | ||
| match self { | ||
| Model::BertTiny => "bert_tiny_tokenizer.json", | ||
| Model::BertTinyV1 => "bert_tiny_tokenizer.json", |
There was a problem hiding this comment.
will both v1 and v2 use the same tokenizer?
Remove the NLD-related feature flags and fasttext classifier Now that the ONNX/BertTiny classifier is the production default: ## Description <!-- Please remember to add your design buddy onto the PR for review, if it contains any UI changes! --> ## Linked Issue <!-- Link the GitHub issue this PR addresses. Before opening this PR, please confirm: --> - [ ] The linked issue is labeled `ready-to-spec` or `ready-to-implement`. - [ ] Where appropriate, screenshots or a short video of the implementation are included below (especially for user-visible or UI changes). ## Testing <!-- How did you test this change? What automated tests did you add? If you didn't add any new tests, what's your justification for not adding any? Manual testing is required for changes that can be manually tested, and almost all changes can be manually tested. If your change can be manually tested, please include screenshots or a screen recording that show it working end to end. You can run the app locally using `./script/run` - see WARP.md for more details on how to get set up. --> https://www.loom.com/share/2d0bc3c4834e43928356266039da1b40 - [x] I have manually tested my changes locally with `./script/run` ### Screenshots / Videos <!-- Attach screenshots or a short video demonstrating the change, where appropriate. Remove this section if it is not relevant to your PR. --> ## Agent Mode - [x] Warp Agent Mode - This PR was created via Warp's AI Agent Mode <!-- ## Changelog Entries for Stable The entries below will be used when constructing a soft-copy of the stable release changelog. Leave blank or remove the lines if no entry in the stable changelog is needed. Entries should be on the same line, without the `{{` `}}` brackets. You can use multiple lines, even of the same type. The valid suffixes are: - NEW-FEATURE: for new, relatively sizable features. Features listed here will likely have docs / social media posts / marketing launches associated with them, so use sparingly. - IMPROVEMENT: for new functionality of existing features. - BUG-FIX: for fixes related to known bugs or regressions. - IMAGE: the image specified by the URL (hosted on GCP) will be added to Dev & Preview releases. For Stable releases, see the pinned doc in the #release Slack channel. - OZ: Oz-related updates. Use `CHANGELOG-OZ`. At most 4 Oz updates are shown in-app per release. CHANGELOG-NEW-FEATURE: {{text goes here...}} CHANGELOG-IMPROVEMENT: {{text goes here...}} CHANGELOG-BUG-FIX: {{text goes here...}} CHANGELOG-BUG-FIX: {{more text goes here...}} CHANGELOG-IMAGE: {{GCP-hosted URL goes here...}} CHANGELOG-OZ: {{text goes here...}} --> ## Reviewers @vorporeal @szgupta --------- Co-authored-by: Oz <oz-agent@warp.dev>
## Description Remove the stale `nld_improvements` Cargo feature from the Nix package build. `nld_improvements` was removed in #10671, but the flake still passed it through `buildFeatures`. That causes `nix run .#warp-terminal-experimental` to fail during Cargo feature validation. Closes #10979. ## Linked Issue <!-- Link the GitHub issue this PR addresses. Before opening this PR, please confirm: --> - [ ] The linked issue is labeled `ready-to-spec` or `ready-to-implement`. - [ ] Where appropriate, screenshots or a short video of the implementation are included below (especially for user-visible or UI changes). ## Testing <!-- How did you test this change? What automated tests did you add? If you didn't add any new tests, what's your justification for not adding any? Manual testing is required for changes that can be manually tested, and almost all changes can be manually tested. If your change can be manually tested, please include screenshots or a screen recording that show it working end to end. You can run the app locally using `./script/run` - see WARP.md for more details on how to get set up. --> - [ ] I have manually tested my changes locally with `./script/run` Validated locally with `nix run .#warp-terminal-experimental` ### Screenshots / Videos <!-- Attach screenshots or a short video demonstrating the change, where appropriate. Remove this section if it is not relevant to your PR. --> ## Agent Mode - [ ] Warp Agent Mode - This PR was created via Warp's AI Agent Mode <!-- ## Changelog Entries for Stable The entries below will be used when constructing a soft-copy of the stable release changelog. Leave blank or remove the lines if no entry in the stable changelog is needed. Entries should be on the same line, without the `{{` `}}` brackets. You can use multiple lines, even of the same type. The valid suffixes are: - NEW-FEATURE: for new, relatively sizable features. Features listed here will likely have docs / social media posts / marketing launches associated with them, so use sparingly. - IMPROVEMENT: for new functionality of existing features. - BUG-FIX: for fixes related to known bugs or regressions. - IMAGE: the image specified by the URL (hosted on GCP) will be added to Dev & Preview releases. For Stable releases, see the pinned doc in the #release Slack channel. - OZ: Oz-related updates. Use `CHANGELOG-OZ`. At most 4 Oz updates are shown in-app per release. - NONE: Explicitly opt out of changelog inclusion. Use `CHANGELOG-NONE` for PRs that should never appear in the changelog (e.g. refactors, internal tooling, CI changes). This prevents the changelog agent from inferring an entry. CHANGELOG-NEW-FEATURE: {{text goes here...}} CHANGELOG-IMPROVEMENT: {{text goes here...}} CHANGELOG-BUG-FIX: {{text goes here...}} CHANGELOG-BUG-FIX: {{more text goes here...}} CHANGELOG-IMAGE: {{GCP-hosted URL goes here...}} CHANGELOG-OZ: {{text goes here...}} CHANGELOG-NONE -->
Remove the NLD-related feature flags and fasttext classifier Now that the ONNX/BertTiny classifier is the production default: ## Description <!-- Please remember to add your design buddy onto the PR for review, if it contains any UI changes! --> ## Linked Issue <!-- Link the GitHub issue this PR addresses. Before opening this PR, please confirm: --> - [ ] The linked issue is labeled `ready-to-spec` or `ready-to-implement`. - [ ] Where appropriate, screenshots or a short video of the implementation are included below (especially for user-visible or UI changes). ## Testing <!-- How did you test this change? What automated tests did you add? If you didn't add any new tests, what's your justification for not adding any? Manual testing is required for changes that can be manually tested, and almost all changes can be manually tested. If your change can be manually tested, please include screenshots or a screen recording that show it working end to end. You can run the app locally using `./script/run` - see WARP.md for more details on how to get set up. --> https://www.loom.com/share/2d0bc3c4834e43928356266039da1b40 - [x] I have manually tested my changes locally with `./script/run` ### Screenshots / Videos <!-- Attach screenshots or a short video demonstrating the change, where appropriate. Remove this section if it is not relevant to your PR. --> ## Agent Mode - [x] Warp Agent Mode - This PR was created via Warp's AI Agent Mode <!-- ## Changelog Entries for Stable The entries below will be used when constructing a soft-copy of the stable release changelog. Leave blank or remove the lines if no entry in the stable changelog is needed. Entries should be on the same line, without the `{{` `}}` brackets. You can use multiple lines, even of the same type. The valid suffixes are: - NEW-FEATURE: for new, relatively sizable features. Features listed here will likely have docs / social media posts / marketing launches associated with them, so use sparingly. - IMPROVEMENT: for new functionality of existing features. - BUG-FIX: for fixes related to known bugs or regressions. - IMAGE: the image specified by the URL (hosted on GCP) will be added to Dev & Preview releases. For Stable releases, see the pinned doc in the #release Slack channel. - OZ: Oz-related updates. Use `CHANGELOG-OZ`. At most 4 Oz updates are shown in-app per release. CHANGELOG-NEW-FEATURE: {{text goes here...}} CHANGELOG-IMPROVEMENT: {{text goes here...}} CHANGELOG-BUG-FIX: {{text goes here...}} CHANGELOG-BUG-FIX: {{more text goes here...}} CHANGELOG-IMAGE: {{GCP-hosted URL goes here...}} CHANGELOG-OZ: {{text goes here...}} --> ## Reviewers @vorporeal @szgupta --------- Co-authored-by: Oz <oz-agent@warp.dev>
Remove the NLD-related feature flags and fasttext classifier
Now that the ONNX/BertTiny classifier is the production default:
Description
Linked Issue
ready-to-specorready-to-implement.Testing
https://www.loom.com/share/2d0bc3c4834e43928356266039da1b40
./script/runScreenshots / Videos
Agent Mode
Reviewers
@vorporeal @szgupta