Feat/onnx export#7
Open
singjc wants to merge 10 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR adds ONNX export support across redeem-properties and redeem-cli. Candle crates move to a git fork with an onnx-export feature, property models gain export hooks, and the CLI adds a ChangesONNX Export Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CliUser
participant MainRs
participant OnnxRs
participant OnnxExportRs
CliUser->>MainRs: properties to-onnx --model ... --output ...
MainRs->>OnnxRs: run_to_onnx(matches)
OnnxRs->>OnnxExportRs: load_and_export_decoder or load_and_export_full
OnnxExportRs->>OnnxRs: exported Value/OnnxGraph
OnnxRs->>CliUser: save ONNX file, print success
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
redeem-cli/src/properties/onnx.rs (1)
48-51: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winRoot cause of the debug-mode panic lives here.
See the comment on
redeem-cli/src/main.rs(to-onnx subcommand arg list) —"external_data_threshold"is never registered as a CLI argument, so thisget_onecall panics in debug builds on every invocation regardless of--external-data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@redeem-cli/src/properties/onnx.rs` around lines 48 - 51, The panic comes from looking up an unregistered CLI arg in `onnx::properties`; update the `get_one::<usize>("external_data_threshold")` access so it only happens after the `to-onnx` command registers that argument in `main.rs`, or remove the lookup if the option is no longer intended. Make the `onnx.rs` logic consistent with the CLI definition by using the same symbol name for registration and retrieval, and ensure the default path cannot call `get_one` for an absent arg.
🧹 Nitpick comments (4)
redeem-cli/src/main.rs (1)
376-384: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlias pattern arm is unreachable.
Some(("to_onnx", onnx_matches))will never match: clap reports the canonical subcommand name ("to-onnx") inmatches.subcommand()even when invoked via theto_onnxalias. The extra arm is dead code but harmless.♻️ Simplify to just the canonical name
- Some(("to-onnx", onnx_matches)) | Some(("to_onnx", onnx_matches)) => { + Some(("to-onnx", onnx_matches)) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@redeem-cli/src/main.rs` around lines 376 - 384, The subcommand match in main is carrying a dead alias arm because clap returns only the canonical name from matches.subcommand(). Update the match around onnx::run_to_onnx to handle just the canonical "to-onnx" case and remove the unreachable "to_onnx" pattern, keeping the error handling and exit behavior unchanged.redeem-cli/src/properties/onnx.rs (1)
36-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFallback value for
componentnever matches the CLI default.
unwrap_or("decoder")here is dead code today sincemain.rssets.default_value("full")for this arg, soget_onealways returnsSome. If that default is ever changed or the flag becomes optional without a default, behavior would silently diverge from intent. Consider aligning the fallback with the actual default for clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@redeem-cli/src/properties/onnx.rs` around lines 36 - 39, The fallback in the component lookup is out of sync with the CLI default, and the dead-code path can mislead future changes. Update the `component` handling in `onnx.rs` so the fallback matches the actual default configured in `main.rs` for the same argument, and keep the `matches.get_one::<String>("component")` logic consistent with that source of truth.redeem-cli/README.md (1)
111-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocs omit
--deviceand--opsetflags.Both flags exist on the
to-onnxsubcommand (main.rs) but aren't documented in the usage template or argument table here.📝 Suggested doc addition
redeem properties to-onnx \ --model <MODEL_PATH> \ --model_arch <MODEL_ARCH> \ --output <OUTPUT_ONNX> \ [--component full|decoder] \ [--constants <CONSTANTS_YAML>] \ + [--device cpu|cuda|cuda:<index>] \ + [--opset <OPSET_VERSION>] \ [--external-data]| `--constants` | Optional model constants YAML | +| `--device` | Device used to load model tensors (`cpu`, or `cuda`/`cuda:<index>` with the cuda feature) | +| `--opset` | ONNX opset version (default 18) | | `--external-data` | Store initializer bytes in a sidecar `.onnx.data` file |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@redeem-cli/README.md` around lines 111 - 130, The README usage and argument table for the redeem properties to-onnx command are missing the existing --device and --opset options from the to-onnx subcommand. Update the usage block and options table in this section to document both flags, including their purpose and placement alongside the other export arguments, using the to-onnx command and main.rs subcommand definitions as the source of truth.Cargo.toml (1)
14-16: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin the Candle fork to an immutable revision.
These
[patch.crates-io]entries track a mutable branch, so the dependency source can change without a manifest diff. Pin the fork withrev = "<commit-sha>"and apply the same pin to the member manifests that reference this fork directly.Suggested shape
-candle-core = { git = "https://github.com/singjc/candle.git", branch = "add/candle-onnx-export" } +candle-core = { git = "https://github.com/singjc/candle.git", rev = "<pinned-commit-sha>" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Cargo.toml` around lines 14 - 16, The Candle fork is currently referenced by a mutable branch in the [patch.crates-io] entries, so pin the fork to an immutable commit revision instead. Update the candle-core, candle-nn, and candle-transformers entries in Cargo.toml to use a fixed rev, and make the same change in any member manifests that reference these crates directly so all uses of the fork resolve to the same commit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@redeem-cli/src/main.rs`:
- Around line 144-221: The to-onnx command is missing the
external_data_threshold argument that redeem-cli/src/properties/onnx.rs expects
via matches.get_one::<usize>("external_data_threshold"). Add that flag to the
Command::new("to-onnx") builder in redeem-cli/src/main.rs with an appropriate
default/value parser, or change the ONNX export code to use
DEFAULT_EXTERNAL_DATA_THRESHOLD directly so the undeclared-id assertion is
avoided.
In `@redeem-properties/src/onnx_export.rs`:
- Around line 271-274: The hidden-dimension validation in the ONNX export path
is tautological and never rejects invalid widths. Update the check in the
mod-embedding export logic around the expected dimension calculation to
explicitly validate that mod_hidden_dim is greater than k and that the resulting
transformed weight/output width matches the intended graph shape. Keep the
existing InvalidGraph error path in this validation branch so invalid
configurations are caught before graph construction.
- Around line 139-141: The exported transformer shape is inconsistent because
export_transformer uses a dynamic seq in the TF input while the positional
encoding path is fixed to TF_MAX_LEN, which can cause shape mismatches for
longer inputs. Update export_transformer and the related add_*_tf_input setup so
the exported sequence length is bounded to TF_MAX_LEN, either by making the
exported seq fixed to 100 or by explicitly enforcing/documenting truncation
before export, and keep the positional encoding and input tensor dimensions
aligned.
---
Duplicate comments:
In `@redeem-cli/src/properties/onnx.rs`:
- Around line 48-51: The panic comes from looking up an unregistered CLI arg in
`onnx::properties`; update the `get_one::<usize>("external_data_threshold")`
access so it only happens after the `to-onnx` command registers that argument in
`main.rs`, or remove the lookup if the option is no longer intended. Make the
`onnx.rs` logic consistent with the CLI definition by using the same symbol name
for registration and retrieval, and ensure the default path cannot call
`get_one` for an absent arg.
---
Nitpick comments:
In `@Cargo.toml`:
- Around line 14-16: The Candle fork is currently referenced by a mutable branch
in the [patch.crates-io] entries, so pin the fork to an immutable commit
revision instead. Update the candle-core, candle-nn, and candle-transformers
entries in Cargo.toml to use a fixed rev, and make the same change in any member
manifests that reference these crates directly so all uses of the fork resolve
to the same commit.
In `@redeem-cli/README.md`:
- Around line 111-130: The README usage and argument table for the redeem
properties to-onnx command are missing the existing --device and --opset options
from the to-onnx subcommand. Update the usage block and options table in this
section to document both flags, including their purpose and placement alongside
the other export arguments, using the to-onnx command and main.rs subcommand
definitions as the source of truth.
In `@redeem-cli/src/main.rs`:
- Around line 376-384: The subcommand match in main is carrying a dead alias arm
because clap returns only the canonical name from matches.subcommand(). Update
the match around onnx::run_to_onnx to handle just the canonical "to-onnx" case
and remove the unreachable "to_onnx" pattern, keeping the error handling and
exit behavior unchanged.
In `@redeem-cli/src/properties/onnx.rs`:
- Around line 36-39: The fallback in the component lookup is out of sync with
the CLI default, and the dead-code path can mislead future changes. Update the
`component` handling in `onnx.rs` so the fallback matches the actual default
configured in `main.rs` for the same argument, and keep the
`matches.get_one::<String>("component")` logic consistent with that source of
truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c72834ef-1e4a-41b8-b461-008d173056f0
📒 Files selected for processing (17)
Cargo.tomlredeem-cli/Cargo.tomlredeem-cli/README.mdredeem-cli/src/main.rsredeem-cli/src/properties/mod.rsredeem-cli/src/properties/onnx.rsredeem-cli/tests/cli_smoke.rsredeem-properties/Cargo.tomlredeem-properties/assets/pretrained_models/redeem/redeem_ccs_cnn_tf.onnxredeem-properties/assets/pretrained_models/redeem/redeem_rt_cnn_tf.onnxredeem-properties/src/lib.rsredeem-properties/src/models/ccs_cnn_lstm_model.rsredeem-properties/src/models/ccs_cnn_tf_model.rsredeem-properties/src/models/ms2_bert_model.rsredeem-properties/src/models/rt_cnn_lstm_model.rsredeem-properties/src/models/rt_cnn_transformer_model.rsredeem-properties/src/onnx_export.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@redeem-properties-py/Cargo.toml`:
- Line 16: The Candle dependency is still tracking a moving git branch, so
update the `candle-core` dependency in `Cargo.toml` to a fixed commit revision
instead of `add/candle-onnx-export`, and make sure the same pinned revision is
used consistently in the matching workspace `Cargo.toml` entries. Use the
existing `candle-core` dependency declaration as the place to change, and keep
all Candle-related package entries aligned to the same commit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f496c13-459f-457b-a607-d3fb5a8af1fb
📒 Files selected for processing (6)
redeem-cli/src/main.rsredeem-openms-ffi/Cargo.tomlredeem-properties-py/Cargo.tomlredeem-properties/assets/pretrained_models/redeem/redeem_ccs_cnn_tf.onnxredeem-properties/assets/pretrained_models/redeem/redeem_rt_cnn_tf.onnxredeem-properties/src/onnx_export.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- redeem-cli/src/main.rs
- redeem-properties/src/onnx_export.rs
| pyo3 = { version = "0.22", features = ["extension-module"] } | ||
| numpy = "0.22" | ||
| anyhow = "1.0" | ||
| candle-core = { git = "https://github.com/singjc/candle.git", branch = "add/candle-onnx-export" } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files '**/Cargo.toml' | sed -n '1,120p'
printf '\n== redeem-properties-py/Cargo.toml ==\n'
cat -n redeem-properties-py/Cargo.toml
printf '\n== Search singjc/candle.git ==\n'
rg -n --hidden --glob 'Cargo.toml' 'singjc/candle\.git|add/candle-onnx-export|rev\s*=' .Repository: singjc/redeem
Length of output: 2772
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Workspace manifests mentioning candle ==\n'
rg -n --hidden --glob 'Cargo.toml' 'candle-core|candle-nn|singjc/candle\.git|add/candle-onnx-export' redeem-properties-py redeem-openms-ffi . || true
printf '\n== Top-level manifest(s) ==\n'
fd -a -t f '^Cargo\.toml$' .
printf '\n== redeem-openms-ffi/Cargo.toml ==\n'
cat -n redeem-openms-ffi/Cargo.tomlRepository: singjc/redeem
Length of output: 4118
Pin the Candle fork to a commit
redeem-properties-py/Cargo.toml still depends on https://github.com/singjc/candle.git via the moving add/candle-onnx-export branch. Pin this to a commit and update the matching Cargo.toml entries in the workspace to the same revision.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@redeem-properties-py/Cargo.toml` at line 16, The Candle dependency is still
tracking a moving git branch, so update the `candle-core` dependency in
`Cargo.toml` to a fixed commit revision instead of `add/candle-onnx-export`, and
make sure the same pinned revision is used consistently in the matching
workspace `Cargo.toml` entries. Use the existing `candle-core` dependency
declaration as the place to change, and keep all Candle-related package entries
aligned to the same commit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request adds ONNX export support to the
redeemCLI and underlying libraries, enabling users to export supported property models (full models or decoder-only) to ONNX format. It introduces new dependencies, a CLI subcommand, and comprehensive documentation and tests around this feature.The most important changes are:
ONNX Export Feature
to-onnxsubcommand to theredeem propertiesCLI, allowing users to export supported property models to ONNX, with options for full-model or decoder/head-only export, device selection, opset version, and external data handling. (redeem-cli/src/main.rs,redeem-cli/src/properties/onnx.rs,redeem-cli/src/properties/mod.rs) [1] [2] [3] [4]redeem-properties(behind theonnx-exportfeature flag), supportingrt_cnn_tfandccs_cnn_tffull-model export, and decoder export for other architectures. (redeem-properties/src/lib.rs,redeem-properties/Cargo.toml,redeem-cli/Cargo.toml) [1] [2] [3]Dependency and Feature Management
candle-core,candle-nn,candle-transformers, and addedcandle-onnx-exportfrom a fork with ONNX export support. Feature flags and optional dependencies are set up accordingly in bothredeem-propertiesandredeem-cli. (Cargo.toml,redeem-properties/Cargo.toml,redeem-cli/Cargo.toml) [1] [2] [3] [4]Documentation and Testing
redeem-cli/README.mddescribing the new ONNX export functionality, with usage examples for both full-model and decoder export.to-onnxsubcommand and its arguments. (redeem-cli/tests/cli_smoke.rs)These changes provide a robust and user-friendly way to export property models to ONNX, facilitating interoperability and deployment in other environments.
Summary by CodeRabbit
properties to-onnxCLI command supporting both full-model and decoder/head-only ONNX exports.