chore: cleanup code to prepare for new features #25
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughCI workflows remove Raven checkout/pinning and instead lock Dune packages and install LLVM and audio libs; OCaml matrix narrowed to 5.3.0. Dune/opam files update OCaml and Dune constraints and add top-level pins referencing raven-ml/raven. Multiple benchmark scripts and executables for read and STFT (Python and OCaml) are deleted. STFT implementation in src/transform.ml refactored to a vmapped per-frame pipeline with rank-aware centering and explicit dtype handling. Utils.frame and its public signature and tests are removed. .gitignore extended with dev-tools.locks, databench/, and .DS_Store. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Transform as Transform.stft
participant Tensor as Rune
Caller->>Transform: stft(x, n_fft, hop_length, win_length, center, window)
Transform->>Tensor: dtype(x), ndim(x)
alt center = true
Transform->>Tensor: pad last axis (pad_width)
end
Transform->>Tensor: build/pad window with dtype
Transform->>Transform: compute num_frames and frame_indices
loop vmapped per-frame (for each idx)
Transform->>Tensor: slice padded input at start = idx * hop_length (length n_fft)
Transform->>Tensor: reshape/preserve leading dims
Transform->>Tensor: apply window
Transform->>Tensor: rfft(last_axis)
Transform-->>Transform: collect frame spectrum
end
Transform->>Tensor: transpose to (..., freq, time)
Transform-->>Caller: return STFT tensor
Note right of Transform: Errors if hop_length <= 0 or n_fft > signal_length
Estimated code review effort4 (Complex) — ~60 minutes PoemI nibbled old benches, byte by byte, Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🪛 YAMLlint (1.37.1).github/workflows/test.yml[warning] 55-55: too few spaces before comment: expected 2 (comments) .github/workflows/build.yml[warning] 43-43: too few spaces before comment: expected 2 (comments) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (3)
Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR performs a code cleanup to prepare for new features by removing deprecated functionality and updating dependencies.
Key changes include:
- Removal of legacy framing functionality and associated tests from the codebase
- Update of dependencies and build configuration for better compatibility
- Simplification of STFT implementation to use vmap instead of removed frame utility
Reviewed Changes
Copilot reviewed 18 out of 27 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_utils.ml | Removed slice_ranges helper, Test_unwrap module, and Test_frame module with all associated test cases |
| test/dune | Commented out test_stft test configuration |
| src/utils.mli | Removed frame function signature and documentation |
| src/utils.ml | Removed frame function implementation |
| src/transform.ml | Refactored STFT implementation to use vmap-based framing instead of removed frame utility |
| soundml.opam | Updated OCaml version requirement and dune version |
| dune-project | Added dependency pins and updated version requirements |
| bench/stft/* | Removed all STFT benchmarking files |
| bench/read/* | Removed all file reading benchmarking utilities |
| .github/workflows/* | Updated CI workflows to use dune pkg lock instead of manual raven pinning |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
.github/workflows/test.yml (2)
20-25: Remove OCaml 5.2.0 from the test matrix.The project requires OCaml >= 5.3.0; testing 5.2.0 is misleading and can break future publishing.
- ocaml-compiler: - - "5.3.0" # Latest stable - - "5.2.0" + ocaml-compiler: + - "5.3.0" # Baseline per opam/dune-project
66-71: Fix typo in step name (“converage” → “coverage”).- - name: Run tests (and send converage to coveralls) + - name: Run tests (and send coverage to Coveralls).github/workflows/build.yml (1)
18-21: Remove OCaml 5.2.0 from the build matrix.Keep CI consistent with the OCaml >= 5.3.0 requirement.
- - "5.3.0" # Latest stable - - "5.2.0" + - "5.3.0" # Baseline per opam/dune-projecttest/test_utils.ml (1)
84-97: Make exception assertions less brittle (avoid matching exact message).Relying on the exact
Invalid_argumentstring risks spurious failures if wording changes.- let expected_exn = - Invalid_argument - "An error occured while trying to pad: current_size > target_size" - in - Alcotest.check_raises - "error_target_too_small: raises Invalid_argument when target_size < \ - input_size" - expected_exn (fun () -> - ignore (Utils.pad_center input_data ~size:target_size ~pad_value) ) + Alcotest.check_raises + "raises Invalid_argument when target_size < input_size" + (Invalid_argument "") + (fun () -> + try + ignore (Utils.pad_center input_data ~size:target_size ~pad_value) + with + | Invalid_argument _ -> raise (Invalid_argument "") + | exn -> raise exn)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (8)
.DS_Storeis excluded by!**/.DS_Storebench/.DS_Storeis excluded by!**/.DS_Storebench/read/both.pngis excluded by!**/*.pngbench/read/mp3.pngis excluded by!**/*.pngbench/read/wav.pngis excluded by!**/*.pngbench/stft/stft_comparison_float32.svgis excluded by!**/*.svgbench/stft/stft_comparison_float64.svgis excluded by!**/*.svgbench/stft/wav_stereo_44100hz_1s.wavis excluded by!**/*.wav
📒 Files selected for processing (19)
.github/workflows/build.yml(1 hunks).github/workflows/lint.yml(1 hunks).github/workflows/test.yml(1 hunks).gitignore(2 hunks)bench/read/bench.py(0 hunks)bench/read/bench_read.sh(0 hunks)bench/read/dune(0 hunks)bench/read/perf.ml(0 hunks)bench/stft/.gitignore(0 hunks)bench/stft/dune(0 hunks)bench/stft/perf.ml(0 hunks)bench/stft/perf.py(0 hunks)dune-project(2 hunks)soundml.opam(1 hunks)src/transform.ml(2 hunks)src/utils.ml(0 hunks)src/utils.mli(0 hunks)test/dune(1 hunks)test/test_utils.ml(1 hunks)
💤 Files with no reviewable changes (10)
- bench/stft/perf.ml
- bench/read/dune
- bench/stft/dune
- bench/stft/.gitignore
- bench/stft/perf.py
- bench/read/perf.ml
- bench/read/bench_read.sh
- src/utils.ml
- bench/read/bench.py
- src/utils.mli
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build (ubuntu-latest, 5.2.0)
- GitHub Check: test (ubuntu-latest, 5.3.0)
- GitHub Check: build (ubuntu-latest, 5.3.0)
- GitHub Check: test (ubuntu-latest, 5.2.0)
- GitHub Check: lint-fmt
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (11)
.github/workflows/lint.yml (1)
21-24: OCaml 5.3 bump matches project—LGTM..github/workflows/test.yml (1)
56-58: Dune locking step—LGTM.Good move to lock dependencies before running tests.
Please confirm lockdir usage is stable with Dune 3.17 on GH Actions (no local opam switch leakage).
.github/workflows/build.yml (1)
41-48: Lock + build flow—LGTM.Locking deps then building is a good replacement for manual Raven pinning.
test/test_utils.ml (1)
447-448: Updated test suite list—LGTM.Runner cleanly lists remaining suites after removing unwrap/frame.
dune-project (1)
18-25: Verify Dune pin usage for nx/rune is effective in CI.No dune.lock found in the repo — cannot confirm CI resolves nx/rune to the pinned Raven git source. Ensure CI either commits the dune.lock produced by
dune pkg lockor runsdune pkg lockduring builds, and verify dune.lock contains url entries pointing to git+https://github.com/raven-ml/raven.git for packages nx and rune (pins in dune-project, ~lines 19–24).src/transform.ml (6)
26-26: Clearer hop_length validation — LGTMMessage and check are correct and consistent with other validations.
31-38: Window dtype/device handling and symmetric pad to n_fft — LGTMUsing
dtype = Rune.dtype xand padding the window ton_fftis correct; symmetric split with remainder on the right is fine.
41-49: Rank‑aware centering pad on last axis — looks good; add a minimal guardAssume
xalways has rank ≥ 1. If not guaranteed, guard with an earlyinvalid_argto avoidpadding.(rank - 1)on rank=0.Would you like me to add a one‑line rank check?
60-76: Verify Rune.take parameter order and Rune.reshape(-1) semantics
- Location: src/transform.ml:60-76.
- Repo search returned no matches for
Rune.takeorRune.reshape(... -1), so assumptions in the original comment could not be confirmed.- Confirm both:
Rune.takeparameter order and accepted index dtype (is the call formRune.take x ~axis indices?).Rune.reshapesupport for-1as a sentinel.- If either assumption is false, remove the unsqueeze/reshape approach and gather a 1‑D frame per map using explicit indices (start = frame_index * hop_length; offsets = arange(0, n_fft); indices = start + offsets; frame = Rune.take x_padded ~axis:(-1) indices; then window and rfft).
78-84: Axis rotation correct — keep transpose (no Rune.moveaxis found)Rotation to (..., freq, time) is correct. Repo search for "moveaxis" / "Rune.moveaxis" returned no matches — keep the explicit transpose in src/transform.ml (lines 78–84) or add a small local moveaxis helper for clarity.
56-59: Confirm Rune.arange dtype or pass int32 endpointsPotential int32 mismatch in arange endpoints — rg found no other
Rune.arangeusages; confirm whetherRune.arangeexpects int32 arguments. If it does, replace with int32 literals to avoid implicit casts:File: src/transform.ml (lines 56-59)
- let frame_indices = Rune.arange device Rune.int32 0 num_frames 1 in + let frame_indices = + Rune.arange device Rune.int32 0l (Int32.of_int num_frames) 1l + in
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build.yml(2 hunks).github/workflows/test.yml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/build.yml
[warning] 40-40: too few spaces before comment: expected 2
(comments)
.github/workflows/test.yml
[warning] 51-51: too few spaces before comment: expected 2
(comments)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test (ubuntu-latest, 5.3.0)
- GitHub Check: build (ubuntu-latest, 5.3.0)
- GitHub Check: lint-fmt
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (4)
.github/workflows/build.yml (2)
19-19: OCaml 5.3 pin looks good.Aligns with dune-project/opam constraints; simplifies the matrix.
48-51: Build step LGTM.Straightforward dune build after locking. Consider @install or a release profile later if needed.
.github/workflows/test.yml (2)
20-20: OCaml 5.3 pin looks good.Consistent with build/lint workflows and project constraints.
51-54: Make LLVM install robust: add apt-get update -y and prefer generic packages or apt.llvm.org for LLVM 20Current explicit llvm-20 package install is fragile on ubuntu-latest — run apt-get update and use version-agnostic packages (llvm-dev libllvm-ocaml-dev) or add the official apt.llvm.org repo if LLVM 20 is mandatory.
File: .github/workflows/test.yml (lines 51-54)
- - name: Install llvm # required for rune - run: | - sudo apt-get install libllvm-20-ocaml-dev libllvm20 llvm-20 llvm-20-dev llvm-20-doc llvm-20-examples llvm-20-runtime + - name: Install llvm # required for rune + run: | + sudo apt-get update + sudo apt-get install -y llvm-dev libllvm-ocaml-devIf LLVM 20 is mandatory, add apt.llvm.org repository setup (can provide snippet).
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 18 out of 27 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build.yml (1)
52-55: Install opam dependencies before running dune builddune pkg lock was run earlier but doesn't install packages; add deps-only install to avoid CI build failures.
File: .github/workflows/build.yml (lines 52–54)
- name: Build SoundML - run: opam exec -- dune build + run: | + opam install -y . --deps-only + opam exec -- dune build shell: bash
♻️ Duplicate comments (4)
.github/workflows/test.yml (2)
55-58: Harden LLVM install; fix yamllint inline-comment spacing.llvm-20 packages may be unavailable on ubuntu-latest; prefer version-agnostic packages and ensure apt-get update/-y. Also add two spaces before the inline comment.
Apply:
- - name: Install llvm # required for rune - run: | - sudo apt-get install libllvm-20-ocaml-dev libllvm20 llvm-20 llvm-20-dev llvm-20-doc llvm-20-examples llvm-20-runtime + - name: Install llvm # required for rune + run: | + sudo apt-get update + sudo apt-get install -y llvm-dev libllvm-ocaml-dev shell: bashIf LLVM 20 is truly required for rune, add apt.llvm.org and then install llvm-20; I can provide a hardened snippet.
63-65: Install dune before locking packages (prevents “command not found”).The switch may not include dune by default; install it explicitly before dune pkg lock.
Apply:
+ - name: Install dune (for locking) + run: opam install -y dune + shell: bash - name: Lock dune packages run: opam exec -- dune pkg lock shell: bash.github/workflows/build.yml (2)
44-47: Make LLVM install robust; fix yamllint spacing.Prefer version-agnostic packages (or add apt.llvm.org if you truly need 20). Also add two spaces before the comment.
Apply:
- - name: Install llvm # required for rune - run: | - sudo apt-get install libllvm-20-ocaml-dev libllvm20 llvm-20 llvm-20-dev llvm-20-doc llvm-20-examples llvm-20-runtime + - name: Install llvm # required for rune + run: | + sudo apt-get update + sudo apt-get install -y llvm-dev libllvm-ocaml-dev shell: bashIf LLVM 20 is mandatory, say the word and I’ll drop in the apt.llvm.org flow.
48-50: Install dune before running dune pkg lock.Prevents failures when dune isn’t in the switch.
Apply:
+ - name: Install dune (for locking) + run: opam install -y dune + shell: bash - name: Lock the dependencies run: opam exec -- dune pkg lock shell: bash
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build.yml(1 hunks).github/workflows/test.yml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/build.yml
[warning] 44-44: too few spaces before comment: expected 2
(comments)
.github/workflows/test.yml
[warning] 55-55: too few spaces before comment: expected 2
(comments)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test (ubuntu-latest, 5.3.0)
- GitHub Check: build (ubuntu-latest, 5.3.0)
- GitHub Check: lint-fmt
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
.github/workflows/test.yml (1)
20-20: OCaml 5.3 matrix looks good.Aligns CI with dune/opam constraints mentioned elsewhere. No action needed.
.github/workflows/build.yml (1)
19-19: OCaml 5.3 matrix update looks good.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
.github/workflows/build.yml (4)
43-43: Fix YAML comment spacing (yamllint).Apply:
- - name: Install llvm # required for rune + - name: Install llvm # required for rune
47-48: Install dune before locking.
opam exec -- dune ...fails if dune isn’t in the switch.Apply:
+ - name: Install dune (for locking) + run: opam install -y dune + shell: bash - name: Lock the dependencies run: opam exec -- dune pkg lock shell: bash
39-42: Use correct dev packages and non-interactive install.Install the right libsndfile dev package and add update/-y to avoid prompts.
Apply:
- - name: Install libsndfile and libsamplerate - run: | - sudo apt-get install libsndfile-dev libsamplerate0-dev + - name: Install libsndfile and libsamplerate + run: | + sudo apt-get update + sudo apt-get install -y libsndfile1-dev libsamplerate0-dev + shell: bash
43-46: LLVM install likely to fail on ubuntu-latest; add update and avoid hardcoding -20.Prefer version-agnostic packages unless you add apt.llvm.org. Also fix non-interactive install.
Apply one of:
Option A (recommended, version-agnostic):
- - name: Install llvm # required for rune - run: | - sudo apt-get install libllvm-20-ocaml-dev libllvm20 llvm-20 llvm-20-dev llvm-20-doc llvm-20-examples llvm-20-runtime + - name: Install llvm # required for rune + run: | + sudo apt-get update + sudo apt-get install -y llvm-dev libllvm-ocaml-dev + shell: bashOption B (if LLVM 20 is mandatory): add apt.llvm.org (key + repo) before installing llvm-20 packages; I can provide a hardened snippet if needed.
.github/workflows/test.yml (3)
55-55: Fix YAML comment spacing (yamllint).- - name: Install llvm # required for rune + - name: Install llvm # required for rune
63-64: Install dune before locking.Prevents “dune: command not found”.
Apply:
+ - name: Install dune (for locking) + run: opam install -y dune + shell: bash - name: Lock dune packages run: opam exec -- dune pkg lock shell: bash
51-53: Use correct dev packages and non-interactive install.Same fix as build workflow.
Apply:
- - name: Install libsndfile and libsamplerate - run: | - sudo apt-get install libsndfile-dev libsamplerate0-dev + - name: Install libsndfile and libsamplerate + run: | + sudo apt-get update + sudo apt-get install -y libsndfile1-dev libsamplerate0-dev + shell: bash
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build.yml(1 hunks).github/workflows/test.yml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/test.yml
[warning] 55-55: too few spaces before comment: expected 2
(comments)
.github/workflows/build.yml
[warning] 43-43: too few spaces before comment: expected 2
(comments)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (ubuntu-latest, 5.3.0)
- GitHub Check: lint-fmt
- GitHub Check: test (ubuntu-latest, 5.3.0)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (3)
.github/workflows/build.yml (2)
19-19: OCaml 5.3.0 matrix looks good.Matches dune-project/opam constraints from this PR.
51-53: Build step LGTM..github/workflows/test.yml (1)
20-20: OCaml 5.3.0 matrix looks good.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 18 out of 27 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Summary by CodeRabbit
Refactor
Chores
Tests