Make rust-ml repository publish-ready and pedagogy-focused - #8
Merged
Conversation
|
Important Review skippedToo many files! This PR contains 234 files, which is 84 over the limit of 150. To get a review, narrow the scope: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (234)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive, CPU-first Rust curriculum equivalent to Stanford's CS336 language modeling course, adding structured assignments, a public content boundary, and several active, tested teaching crates. The code review feedback is highly constructive, pointing out performance optimization opportunities in the attention and mlp crates by zipping iterators directly to avoid unnecessary heap allocations. Additionally, it identifies a logical redundancy in the inference crate's temperature-scaled top-k sampling, where pre-sorted rankings make the search loop unnecessary.
Comment on lines
+869
to
+878
| } | ||
|
|
||
| let token_values = token.values().copied().collect::<Vec<_>>(); | ||
| let mut output = Vec::with_capacity(self.shape.rows().as_usize()); | ||
| for row in &self.rows { | ||
| let mut sum = ProjectionOutput::zero()?; | ||
| for (component, weight) in token_values.iter().zip(row.values()) { | ||
| let product = (*component * *weight)?; | ||
| sum = (sum + product)?; | ||
| } |
There was a problem hiding this comment.
Avoid copying and collecting the vector values into a new heap-allocated Vec in project. You can zip the iterator directly to avoid unnecessary allocations.
let mut output = Vec::with_capacity(self.shape.rows().as_usize());
for row in &self.rows {
let mut sum = ProjectionOutput::zero()?;
for (component, weight) in token.values().copied().zip(row.values()) {
let product = (component * *weight)?;
sum = (sum + product)?;
}
output.push(sum);
}
Comment on lines
+307
to
+318
| } | ||
| } | ||
|
|
||
| /// Hidden representation after ReLU. | ||
| #[derive(Debug, Clone, PartialEq)] | ||
| pub struct HiddenActivation(DenseVector<HiddenActivationValue>); | ||
|
|
||
| impl HiddenActivation { | ||
| fn from_values( | ||
| values: impl IntoIterator<Item = HiddenActivationValue>, | ||
| ) -> Result<Self, MlpError> { | ||
| Ok(Self(DenseVector::from_values( |
There was a problem hiding this comment.
Avoid copying and collecting the vector values into a new heap-allocated Vec in multiply_input and multiply_hidden. Since DenseVector is defined in the same module, you can directly reference the inner values slice (&vector.0.values) to avoid unnecessary allocations.
fn multiply_input(&self, vector: &InputVector) -> Result<DenseVector<WeightedSum>, MlpError> {
self.multiply_values("WeightMatrix::multiply_input", vector.width(), &vector.0.values)
}
fn multiply_hidden(
&self,
vector: &HiddenActivation,
) -> Result<DenseVector<WeightedSum>, MlpError> {
self.multiply_values("WeightMatrix::multiply_hidden", vector.width(), &vector.0.values)
}
Comment on lines
+357
to
+379
| fn try_from(value: usize) -> Result<Self, Self::Error> { | ||
| Ok(Self(nonzero_usize( | ||
| "context window must be greater than zero", | ||
| "ContextWindow::try_from", | ||
| value, | ||
| )?)) | ||
| } | ||
| } | ||
|
|
||
| impl fmt::Display for ContextWindow { | ||
| fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| write!(formatter, "{} tokens", self.0) | ||
| } | ||
| } | ||
|
|
||
| /// Total token budget implied by a prompt and generation request. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] | ||
| pub struct TokenBudget(usize); | ||
|
|
||
| impl TokenBudget { | ||
| fn from_raw(operation: &'static str, value: usize) -> Result<Self, InferenceError> { | ||
| Ok(Self(nonzero_usize( | ||
| "token budget must be greater than zero", |
There was a problem hiding this comment.
Since rankings are already sorted in descending order (via compare_ranked_tokens) and temperature is strictly positive, division by temperature preserves the relative order of the logits. Consequently, the first candidate in the iteration will always have the maximum adjusted logit, making the loop and division redundant. If deterministic selection is intended for this toy implementation, you can simplify this to directly return the first candidate.
Self::TopK { .. } => {
rankings.candidates()
.next()
.map(|candidate| candidate.token_id())
.ok_or_else(|| InferenceError::empty_input(
"SamplingMode::select",
"top-k candidate set cannot be empty",
))
}
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.
Summary
Validation