From 202e27c75d3665abbf5223d857b8f48e63fbb52c Mon Sep 17 00:00:00 2001 From: mikemikimike <13286568797@163.com> Date: Sun, 13 Sep 2026 23:08:02 +0800 Subject: [PATCH 1/2] fix: prevent neural network layer parameter aliasing Signed-off-by: mikemikimike <13286568797@163.com> --- crates/oxmera-nn/src/conv.rs | 18 ++++++++++++- crates/oxmera-nn/src/embedding.rs | 13 ++++++++- crates/oxmera-nn/src/linear.rs | 13 ++++++++- crates/oxmera-nn/src/param.rs | 14 +++++++++- crates/oxmera-nn/tests/layers.rs | 44 +++++++++++++++++++++++++++++++ 5 files changed, 98 insertions(+), 4 deletions(-) diff --git a/crates/oxmera-nn/src/conv.rs b/crates/oxmera-nn/src/conv.rs index 496d70c..46f59e5 100644 --- a/crates/oxmera-nn/src/conv.rs +++ b/crates/oxmera-nn/src/conv.rs @@ -11,7 +11,7 @@ use crate::{Module, init}; /// 2-D convolution: input `[n, c_in, h, w]`, output /// `[n, c_out, h_out, w_out]`. -#[derive(Debug, Clone)] +#[derive(Debug)] pub struct Conv2d { weight: Param, bias: Option, @@ -48,6 +48,22 @@ impl Conv2d { } } + /// Create an independent copy of this layer and all of its parameters. + /// + /// Parameters are not implicitly cloneable because [`Param::clone`] + /// intentionally preserves parameter sharing for optimizers. + pub fn deep_clone(&self) -> Result { + Ok(Self { + weight: self.weight.detached_copy()?, + bias: self.bias.as_ref().map(Param::detached_copy).transpose()?, + in_channels: self.in_channels, + out_channels: self.out_channels, + kernel: self.kernel, + stride: self.stride, + padding: self.padding, + }) + } + /// The `[c_out, c_in, kh, kw]` weight handle. pub fn weight(&self) -> &Param { &self.weight diff --git a/crates/oxmera-nn/src/embedding.rs b/crates/oxmera-nn/src/embedding.rs index df31f51..9670893 100644 --- a/crates/oxmera-nn/src/embedding.rs +++ b/crates/oxmera-nn/src/embedding.rs @@ -8,7 +8,7 @@ use crate::{Module, init}; /// A learnable lookup table: `I64` indices of any shape in, embeddings of /// shape `indices.shape() + [dim]` out. -#[derive(Debug, Clone)] +#[derive(Debug)] pub struct Embedding { weight: Param, dim: usize, @@ -23,6 +23,17 @@ impl Embedding { } } + /// Create an independent copy of this layer and its parameter table. + /// + /// Parameters are not implicitly cloneable because [`Param::clone`] + /// intentionally preserves parameter sharing for optimizers. + pub fn deep_clone(&self) -> Result { + Ok(Self { + weight: self.weight.detached_copy()?, + dim: self.dim, + }) + } + /// The `[vocab, dim]` weight handle. pub fn weight(&self) -> &Param { &self.weight diff --git a/crates/oxmera-nn/src/linear.rs b/crates/oxmera-nn/src/linear.rs index f337e4c..2a9c241 100644 --- a/crates/oxmera-nn/src/linear.rs +++ b/crates/oxmera-nn/src/linear.rs @@ -8,7 +8,7 @@ use crate::{Module, init}; /// `y = x Wᵀ + b`: input `[batch, in_features]`, output /// `[batch, out_features]`. -#[derive(Debug, Clone)] +#[derive(Debug)] pub struct Linear { weight: Param, bias: Option, @@ -39,6 +39,17 @@ impl Linear { } } + /// Create an independent copy of this layer and all of its parameters. + /// + /// Parameters are not implicitly cloneable because [`Param::clone`] + /// intentionally preserves parameter sharing for optimizers. + pub fn deep_clone(&self) -> Result { + Ok(Self { + weight: self.weight.detached_copy()?, + bias: self.bias.as_ref().map(Param::detached_copy).transpose()?, + }) + } + /// The `[out, in]` weight handle. pub fn weight(&self) -> &Param { &self.weight diff --git a/crates/oxmera-nn/src/param.rs b/crates/oxmera-nn/src/param.rs index 9a41585..710b243 100644 --- a/crates/oxmera-nn/src/param.rs +++ b/crates/oxmera-nn/src/param.rs @@ -2,13 +2,16 @@ use std::sync::{Arc, RwLock}; +use oxmera_core::Result; use oxmera_tensor::tensor::Tensor; /// A learnable parameter: a shared, replaceable handle to a /// gradient-accumulating leaf tensor. /// /// Modules read the current value each forward pass; optimizers write -/// updated values back through the same handle. +/// updated values back through the same handle. Cloning a `Param` shares +/// that handle intentionally, while [`Param::detached_copy`] creates an +/// independent parameter for explicitly copying a module. #[derive(Debug, Clone)] pub struct Param { inner: Arc>, @@ -22,6 +25,15 @@ impl Param { } } + /// Create an independent parameter with the same current value. + /// + /// Unlike [`Clone::clone`], this copies the tensor storage and starts a + /// fresh gradient-accumulating leaf. The copy remains on the same device + /// and keeps the source tensor's dtype and shape. + pub fn detached_copy(&self) -> Result { + Ok(Self::new(self.value().contiguous_untracked()?)) + } + /// The current value (a cheap clone sharing storage and tape state). pub fn value(&self) -> Tensor { self.inner.read().expect("param lock poisoned").clone() diff --git a/crates/oxmera-nn/tests/layers.rs b/crates/oxmera-nn/tests/layers.rs index 3ddb867..e00720c 100644 --- a/crates/oxmera-nn/tests/layers.rs +++ b/crates/oxmera-nn/tests/layers.rs @@ -218,3 +218,47 @@ fn safetensors_round_trip() { assert_close(&before, &after, 1e-6, "safetensors round trip"); std::fs::remove_file(&path).ok(); } + +#[test] +fn deep_clone_detaches_layer_parameters() { + let linear = Linear::new(2, 2, 1); + linear + .weight() + .set(Tensor::from_slice(&[1.0, 2.0, 3.0, 4.0], [2, 2]).unwrap()); + linear + .bias() + .unwrap() + .set(Tensor::from_slice(&[5.0, 6.0], [2]).unwrap()); + let linear_copy = linear.deep_clone().unwrap(); + linear.weight().set(Tensor::zeros([2, 2])); + linear.bias().unwrap().set(Tensor::zeros([2])); + assert_eq!( + linear_copy.weight().value().to_vec_f32().unwrap(), + vec![1.0, 2.0, 3.0, 4.0] + ); + assert_eq!( + linear_copy.bias().unwrap().value().to_vec_f32().unwrap(), + vec![5.0, 6.0] + ); + + let conv = Conv2d::new(1, 1, (2, 2), 1, 0, 2); + conv.weight() + .set(Tensor::from_slice(&[1.0, 2.0, 3.0, 4.0], [1, 1, 2, 2]).unwrap()); + let conv_copy = conv.deep_clone().unwrap(); + conv.weight().set(Tensor::zeros([1, 1, 2, 2])); + assert_eq!( + conv_copy.weight().value().to_vec_f32().unwrap(), + vec![1.0, 2.0, 3.0, 4.0] + ); + + let embedding = Embedding::new(2, 2, 3); + embedding + .weight() + .set(Tensor::from_slice(&[1.0, 2.0, 3.0, 4.0], [2, 2]).unwrap()); + let embedding_copy = embedding.deep_clone().unwrap(); + embedding.weight().set(Tensor::zeros([2, 2])); + assert_eq!( + embedding_copy.weight().value().to_vec_f32().unwrap(), + vec![1.0, 2.0, 3.0, 4.0] + ); +} From f938633133e8d30bac35a878395b1cf62a1cac04 Mon Sep 17 00:00:00 2001 From: Vyncint Ng <115854244+vyncint@users.noreply.github.com> Date: Sun, 13 Sep 2026 22:24:44 +0700 Subject: [PATCH 2/2] docs(changelog): declare the Clone removal, and pin the copy as trainable Removing `Clone` from `Linear`, `Conv2d` and `Embedding` is a breaking public API change and the CHANGELOG did not say so. Recorded under Changed with the reason, the replacement (`deep_clone`, `Param::detached_copy`), and the note that nothing in the workspace cloned these layers, so the break is confined to out-of-tree callers. The new regression test asserts value independence, which is half the property. A copy that is no longer a gradient-accumulating leaf would pass every one of those assertions and then silently never learn, so a second test pins the leaf flag, that the copy accumulates its own gradient, and that the original does not see it. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com> --- CHANGELOG.md | 14 ++++++++++++++ crates/oxmera-nn/tests/layers.rs | 30 ++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1407a88..b782676 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **`Linear`, `Conv2d` and `Embedding` no longer implement `Clone`.** + Breaking. `Param` is a shared handle by design, so deriving `Clone` on a + layer produced a copy that trained together with the original: setting a + weight on one changed the other, which is weight tying by accident and + had no warning anywhere. Each of the three now has + `deep_clone() -> Result`, which copies the storage and returns an + independent layer, and `Param::detached_copy()` is the same operation one + level down. `Param` itself still clones as a shared handle — optimizers + and `parameters()` depend on that, and it is now documented as + deliberate rather than implied. + + Nothing in the workspace cloned these layers, so the break is confined to + out-of-tree callers; the fix at a call site is `.deep_clone()?`. + - **termlens 0.10.1 → 0.11**, with the vendored skill and the report action's `cli-version:` pins in `ci.yml` and `stress.yml` (`check-skill-version.sh` holds all three equal to the dependency). 0.11 diff --git a/crates/oxmera-nn/tests/layers.rs b/crates/oxmera-nn/tests/layers.rs index e00720c..bf1afb1 100644 --- a/crates/oxmera-nn/tests/layers.rs +++ b/crates/oxmera-nn/tests/layers.rs @@ -262,3 +262,33 @@ fn deep_clone_detaches_layer_parameters() { vec![1.0, 2.0, 3.0, 4.0] ); } + +#[test] +fn a_deep_cloned_layer_is_still_trainable() { + // Value independence is only half of it: a copy that is no longer a + // gradient-accumulating leaf would pass every assertion above and then + // silently never learn. Pin the leaf, and pin that the two layers + // accumulate separately. + let linear = Linear::new(2, 2, 1); + let copy = linear.deep_clone().unwrap(); + assert!( + copy.weight().value().requires_grad(), + "a deep-cloned weight must still be a gradient-accumulating leaf" + ); + + let x = Tensor::from_slice(&[1.0, 1.0], [1, 2]).unwrap(); + copy.forward(&x) + .unwrap() + .sum(&[]) + .unwrap() + .backward() + .unwrap(); + assert!( + copy.weight().grad().is_some(), + "the copy must accumulate its own gradient" + ); + assert!( + linear.weight().grad().is_none(), + "the original must not see the copy's gradient" + ); +}