Merge into working PR.#34
Conversation
There was a problem hiding this comment.
Pull request overview
This PR upgrades the Burn ML framework from version 0.19 to 0.20, which involves significant API changes in the training and inference infrastructure. The changes properly adapt to Burn's new training API architecture while also including a retrained model with updated decision thresholds.
Changes:
- Updated Burn dependency from 0.19 to 0.20 with major API refactoring for training infrastructure
- Refactored
KordBatcherto remove device storage and use the device parameter passed tobatch()method - Migrated from
TrainStep/ValidSteptoTrainStep/InferenceStepwith associated types, and fromLearnerBuildertoSupervisedTrainingAPI - Added new
train-modeltask to Makefile.toml for convenient model training with configurable epochs - Updated model thresholds based on retraining with the new framework
- Minor documentation updates to clarify cargo-make requirements for remote agents
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| kord/Cargo.toml | Updates Burn dependency from 0.19 to 0.20 |
| Cargo.lock | Massive dependency tree update reflecting Burn 0.20's restructuring (burn-common → burn-std, cubecl reorganization) |
| kord/src/ml/train/helpers.rs | Implements new TrainStep/InferenceStep traits with associated types instead of generic parameters |
| kord/src/ml/train/execute.rs | Migrates to SupervisedTraining API, removes LearnerBuilder pattern, adds explicit device transfer after training |
| kord/src/ml/train/data.rs | Simplifies KordBatcher by removing device field, using device parameter from batch() method |
| kord/src/ml/infer/execute.rs | Adds allow(unused_variables) for conditionally-used error variable |
| kord/model/thresholds.json | Updates decision thresholds from retrained model |
| Makefile.toml | Adds train-model task with EPOCHS environment variable support |
| AGENTS.md | Documents cargo-make requirement for remote agents |
| .github/workflows/copilot-setup-steps.yml | Reformats YAML indentation but truncates final build step |
| .github/copilot-instructions.md | Documents cargo-make requirement for remote agents |
| packages: cargo-make cargo-leptos wasm-pack wkg cargo-nextest cargo-llvm-cov leptosfmt | ||
|
|
||
| - name: Prefetch deps | ||
| run: cargo fetch |
There was a problem hiding this comment.
The workflow file appears incomplete. The original version had a "Smoke build (core)" step at the end (line 48-49) that ran "cargo make build", but it was removed in this change. This looks like an accidental truncation rather than an intentional removal. The step should be restored to ensure the workflow validates that the codebase builds correctly.
| run: cargo fetch | |
| run: cargo fetch | |
| - name: Smoke build (core) | |
| run: cargo make build |
| let chords = if pitches.is_empty() { | ||
| vec![] | ||
| } else { | ||
| #[allow(unused_variables)] |
There was a problem hiding this comment.
The #[allow(unused_variables)] annotation is misleading here. The closure parameter e is actually used on line 129 when the cli feature is enabled. This attribute suggests the variable is never used, but it is conditionally used. Consider using #[cfg_attr(not(feature = "cli"), allow(unused_variables))] to more accurately reflect that the variable is only unused when the CLI feature is disabled.
| #[allow(unused_variables)] | |
| #[cfg_attr(not(feature = "cli"), allow(unused_variables))] |
| let model_trained = learner.fit(dataloader_train, dataloader_valid).model; | ||
| let model_trained = training.launch(Learner::new(model, adam_config.init(), lr_scheduler)).model; | ||
|
|
||
| // Move model to device to ensure all weights are on the correct device after training. |
There was a problem hiding this comment.
The addition of to_device(&device) after training (line 111) suggests that the model might end up on a different device after the training process in Burn 0.20. This is a potentially important change. Please verify that this is necessary and document why it's needed, as it could indicate a behavior change in Burn 0.20 where the model device is not guaranteed to be preserved during training. If this is a workaround for a bug or unexpected behavior, consider adding a comment explaining the rationale.
| // Move model to device to ensure all weights are on the correct device after training. | |
| // Defensive: ensure the trained model is on `device` before running inference/evaluation. | |
| // In Burn 0.20, the exact device placement of the model returned from `SupervisedTraining::launch` | |
| // is not explicitly documented, especially when using an `AutodiffBackend` with an `InnerBackend` | |
| // for validation. Calling `to_device(&device)` here guarantees that all parameters end up on the | |
| // same device we used for model construction and for subsequent prediction (`collect_prediction_stats`). | |
| // If Burn already preserves the original device, this call is effectively a no-op, but it protects us | |
| // against subtle device mismatches if that behavior changes. |
No description provided.