test(huntsman-integration): Add neural network tasks. - #377
Conversation
WalkthroughThis change adds two new Rust crates to the workspace: ChangesHuntsman NN example
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
examples/huntsman/nn/tasks/src/lib.rs (1)
5-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce duplication in the NN task wrappers The three task functions repeat the same 25-argument list and hardcode the fan-in separately from
huntsman_nn_core::NUM_INPUTS. A small local macro or a single composite parameter would remove the manual sync point.🤖 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 `@examples/huntsman/nn/tasks/src/lib.rs` around lines 5 - 112, Reduce duplication in the NN task wrappers by removing the repeated 25-argument signatures in dense_relu, dense_sigmoid, and dense_identity and stop hardcoding the fan-in separately from huntsman_nn_core::NUM_INPUTS. Refactor the task_decl module with a small local macro or a single composite input parameter so all three task functions share one source of truth for the input count and stay in sync with huntsman_nn_core::NUM_INPUTS.examples/huntsman/nn/client/src/main.rs (3)
391-398: 🩺 Stability & Availability | 🔵 TrivialNo cleanup of the created resource group after the job finishes.
Each run creates a new resource group (
huntsman-nn-<random>) that is never removed, regardless of job outcome. For an example client intended for repeated validation runs (e.g., against a shared instance), this will accumulate orphaned resource groups over time.🤖 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 `@examples/huntsman/nn/client/src/main.rs` around lines 391 - 398, The Huntsman NN client creates a random resource group via add_resource_group and never removes it, leaving orphaned groups after each run. Update main to clean up the created resource group in a finally-style path after the job completes, using the resource_group_id returned from add_resource_group and ensuring the cleanup runs whether the job succeeds or fails.
432-456: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest only covers
generate_topology, notbuild_graph's wiring translation.
inner_layer_wiring_covers_previous_layervalidates the raw topology coverage guarantee, but doesn't assert thatbuild_graphcorrectly mapsLayer::wiringindices intoTaskInputOutputIndexsources (i.e., thatprev_layer[src]wiring lines up withsimulate'slayer_outputs[sources[k]]). Given the PR objective calls out a test that verifies "the client correctly wires the outputs of inner tasks," a test exercisingbuild_graphagainst a small fixed topology would close this gap.🤖 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 `@examples/huntsman/nn/client/src/main.rs` around lines 432 - 456, The current test only verifies generate_topology coverage and does not exercise build_graph’s wiring translation. Add or update a test around build_graph (and its use of simulate / layer_outputs) with a small fixed topology so it explicitly checks that Layer::wiring indices are mapped into the correct TaskInputOutputIndex sources for inner tasks. Use the existing inner_layer_wiring_covers_previous_layer test as a reference point, but assert the constructed graph’s wiring, not just the raw topology.
90-99: 📐 Maintainability & Code Quality | 🔵 TrivialConsider modelling layer-0 vs. inner layers structurally instead of
Option+.expect().
Layer::wiringbeingOption<Vec<Vec<usize>>>pushes.expect("inner layer wiring is set")into three separate call sites (build_graph,simulate, and implicitly relied upon ingenerate_topology). An enum distinguishingLayer0/Inner(wiring)(or splittingtopology[0]out as its own value at construction) would make the "layer 0 has no wiring" invariant unrepresentable-by-construction and remove the repeated panics.♻️ Sketch of an alternative modelling
enum LayerWiring { Input, Wired(Vec<Vec<usize>>), } struct Layer { activation: Activation, wiring: LayerWiring, }
build_graph/simulatewould thenmatchonLayerWiringinstead of calling.expect(...).Also applies to: 117-143, 157-203, 228-251
🤖 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 `@examples/huntsman/nn/client/src/main.rs` around lines 90 - 99, The `Layer::wiring` design still encodes layer-0 as `Option` and forces repeated `.expect("inner layer wiring is set")` handling in `build_graph`, `simulate`, and `generate_topology`. Refactor `Layer` to make the invariant explicit by introducing a dedicated wiring type such as `LayerWiring::Input` and `LayerWiring::Wired(...)`, or by splitting the first layer out at construction. Update the affected methods to `match` on the new type instead of unwrapping, so the layer-0 vs inner-layer distinction is represented structurally.
🤖 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 `@examples/huntsman/nn/client/src/main.rs`:
- Around line 278-298: The poll loop in poll_until_terminal can run forever
because it has no overall timeout or retry limit. Update poll_until_terminal to
enforce a bounded wait, using a timeout or max-attempts around the existing
get_job_state loop and tokio::time::sleep delay, and return an anyhow error when
the job never reaches a terminal JobState. Keep the fix localized to
poll_until_terminal and any callers that depend on its behavior so CI and
validation runs fail clearly instead of hanging.
---
Nitpick comments:
In `@examples/huntsman/nn/client/src/main.rs`:
- Around line 391-398: The Huntsman NN client creates a random resource group
via add_resource_group and never removes it, leaving orphaned groups after each
run. Update main to clean up the created resource group in a finally-style path
after the job completes, using the resource_group_id returned from
add_resource_group and ensuring the cleanup runs whether the job succeeds or
fails.
- Around line 432-456: The current test only verifies generate_topology coverage
and does not exercise build_graph’s wiring translation. Add or update a test
around build_graph (and its use of simulate / layer_outputs) with a small fixed
topology so it explicitly checks that Layer::wiring indices are mapped into the
correct TaskInputOutputIndex sources for inner tasks. Use the existing
inner_layer_wiring_covers_previous_layer test as a reference point, but assert
the constructed graph’s wiring, not just the raw topology.
- Around line 90-99: The `Layer::wiring` design still encodes layer-0 as
`Option` and forces repeated `.expect("inner layer wiring is set")` handling in
`build_graph`, `simulate`, and `generate_topology`. Refactor `Layer` to make the
invariant explicit by introducing a dedicated wiring type such as
`LayerWiring::Input` and `LayerWiring::Wired(...)`, or by splitting the first
layer out at construction. Update the affected methods to `match` on the new
type instead of unwrapping, so the layer-0 vs inner-layer distinction is
represented structurally.
In `@examples/huntsman/nn/tasks/src/lib.rs`:
- Around line 5-112: Reduce duplication in the NN task wrappers by removing the
repeated 25-argument signatures in dense_relu, dense_sigmoid, and dense_identity
and stop hardcoding the fan-in separately from huntsman_nn_core::NUM_INPUTS.
Refactor the task_decl module with a small local macro or a single composite
input parameter so all three task functions share one source of truth for the
input count and stay in sync with huntsman_nn_core::NUM_INPUTS.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 61b5e0eb-663e-45f6-a79d-5a619d68fae3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlexamples/huntsman/nn/client/Cargo.tomlexamples/huntsman/nn/client/src/main.rsexamples/huntsman/nn/core/Cargo.tomlexamples/huntsman/nn/core/src/lib.rsexamples/huntsman/nn/tasks/Cargo.tomlexamples/huntsman/nn/tasks/src/lib.rs
| /// Periodically polls the job state until it reaches a terminal state. | ||
| /// | ||
| /// # Returns | ||
| /// | ||
| /// The terminal [`JobState`] on success. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Forwards [`SpiderClient::get_job_state`]'s return values on failure. | ||
| async fn poll_until_terminal(client: &SpiderClient, job_id: JobId) -> anyhow::Result<JobState> { | ||
| loop { | ||
| let state = client | ||
| .get_job_state(job_id) | ||
| .await | ||
| .context("get_job_state")?; | ||
| if state.is_terminal() { | ||
| return Ok(state); | ||
| } | ||
| tokio::time::sleep(Duration::from_millis(500)).await; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unbounded polling loop can hang the client forever.
poll_until_terminal loops with no maximum attempt count or overall timeout — if the job never reaches a terminal JobState (e.g., Spider gets stuck or a worker crashes mid-job), this validation client will block indefinitely instead of failing with a clear error. This is especially relevant for CI or automated validation runs mentioned in the PR description.
🛠️ Suggested fix: bound the poll loop
-async fn poll_until_terminal(client: &SpiderClient, job_id: JobId) -> anyhow::Result<JobState> {
- loop {
+async fn poll_until_terminal(
+ client: &SpiderClient,
+ job_id: JobId,
+ max_attempts: usize,
+) -> anyhow::Result<JobState> {
+ for _ in 0..max_attempts {
let state = client
.get_job_state(job_id)
.await
.context("get_job_state")?;
if state.is_terminal() {
return Ok(state);
}
tokio::time::sleep(Duration::from_millis(500)).await;
}
+ Err(anyhow!("timed out waiting for job {} to reach a terminal state", job_id.get()))
}Also applies to: 411-413
🤖 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 `@examples/huntsman/nn/client/src/main.rs` around lines 278 - 298, The poll
loop in poll_until_terminal can run forever because it has no overall timeout or
retry limit. Update poll_until_terminal to enforce a bounded wait, using a
timeout or max-attempts around the existing get_job_state loop and
tokio::time::sleep delay, and return an anyhow error when the job never reaches
a terminal JobState. Keep the fix localized to poll_until_terminal and any
callers that depend on its behavior so CI and validation runs fail clearly
instead of hanging.
| /// (`WEIGHTS[k] = (k + 1) * 0.01 * (-1)^k`), alternating in sign starting positive. | ||
| pub const WEIGHTS: [f64; NUM_INPUTS] = [ | ||
| 0.01, -0.02, 0.03, -0.04, 0.05, -0.06, 0.07, -0.08, 0.09, -0.10, 0.11, -0.12, 0.13, -0.14, | ||
| 0.15, -0.16, 0.17, -0.18, 0.19, -0.20, 0.21, -0.22, 0.23, -0.24, 0.25, | ||
| ]; |
| use spider_tdl::r#std::double; | ||
| use spider_tdl::task; | ||
|
|
||
| #[task(name = "neuron::dense_relu")] |
There was a problem hiding this comment.
Renaming nn -> neuron:
- The package name is already
nn. - These tasks are actual simulated "neuron"s
2 participants
Description
This PR adds in
example/huntsman/nnthree components:core: Neural network neuron functions.tasks: Spider task wrapper for neuron functions.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Summary
New Features
Tests