Stabilize PPO pipeline, tests, and crate release readiness - #29
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 18 minutes and 13 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (50)
📝 WalkthroughWalkthroughThis pull request comprehensively refactors the PPO algorithm implementation to fix critical training bugs, redesigns the SIMD GAE interface to properly separate termination/truncation handling, standardizes divisibility checks across the codebase using Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/buffer/nstep.rs (1)
244-252:⚠️ Potential issue | 🟠 MajorTerminal n-step transition stores the wrong
next_obswhendone=true.At Line 249, the code uses the done transition’s
obsasnext_obs. For terminal transitions,next_obsshould be the terminal successor observation (final_next_obs), not the current state observation. This can corrupt stored transition semantics.💡 Suggested fix
- let (n_step_next_obs, n_step_done) = if encountered_done { - // Episode ended within n-step window, use last valid obs - // The done flag should be true - ( - self.n_step_buffer - .get(actual_n.saturating_sub(1)) - .map(|t| t.obs.clone()) - .unwrap_or_else(|| final_next_obs.to_vec()), - true, - ) + let (n_step_next_obs, n_step_done) = if encountered_done { + // Episode ended within n-step window. + // For terminal transitions, next_obs should be the terminal successor obs. + (final_next_obs.to_vec(), true) } else { // Full n-step return, use the provided next_obs (final_next_obs.to_vec(), final_done) };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/buffer/nstep.rs` around lines 244 - 252, When encountered_done is true the code currently picks the done transition’s obs from self.n_step_buffer as n_step_next_obs; instead use the terminal successor final_next_obs instead. Update the encountered_done branch (where n_step_next_obs and n_step_done are set) to assign n_step_next_obs = final_next_obs.to_vec() (with the same unwrap fallback if needed) and keep n_step_done = true; reference symbols: encountered_done, n_step_next_obs, n_step_done, n_step_buffer, actual_n, final_next_obs.benches/gpu_benchmark.rs (1)
9-18:⚠️ Potential issue | 🔴 CriticalCompilation error: cannot push to immutable
Vec.Line 9 declares
devicesas immutable (let devices), but line 14 attemptsdevices.push(...)which requires mutability. This will fail to compile when themetalfeature is enabled.🐛 Proposed fix
fn get_devices() -> Vec<(&'static str, candle_core::Device)> { - let devices = vec![("CPU", candle_core::Device::Cpu)]; + let mut devices = vec![("CPU", candle_core::Device::Cpu)]; #[cfg(feature = "metal")] {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benches/gpu_benchmark.rs` around lines 9 - 18, The vector `devices` is declared immutable but later mutated; change its declaration to be mutable (e.g., `let mut devices = vec![("CPU", candle_core::Device::Cpu)];`) so that the subsequent `devices.push(("Metal", metal))` inside the `#[cfg(feature = "metal")]` block can compile; locate the `devices` binding and the call to `candle_core::Device::new_metal` to apply this fix.src/simd/mod.rs (1)
382-386:⚠️ Potential issue | 🟡 MinorDeclare an MSRV of 1.73.0 or use an alternative method for alignment checks.
usize::is_multiple_ofwas stabilized in Rust 1.73.0, but the project has no explicit MSRV declared in Cargo.toml. This means the code could fail to compile on older Rust versions. Either addrust-version = "1.73"to Cargo.toml or replace the check withaddr % NEON_ALIGNMENT == 0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/simd/mod.rs` around lines 382 - 386, The alignment check uses usize::is_multiple_of (stabilized in Rust 1.73) which may break on older toolchains; either declare MSRV by adding rust-version = "1.73" to Cargo.toml or change the check in the function that uses addr and NEON_ALIGNMENT (the block returning SimdError::AlignmentError) to use a portable expression: test with addr % NEON_ALIGNMENT == 0 (and keep actual: addr % NEON_ALIGNMENT) instead of is_multiple_of to preserve behavior on older Rust versions.src/networks/attention.rs (1)
50-55:⚠️ Potential issue | 🟠 MajorAdd explicit
num_heads > 0guard beforeis_multiple_ofchecks.Both
SelfAttention::new(line 51) andCrossAttention::new(line 276) haveis_multiple_of(num_heads)assertions that allownum_heads=0whend_model=0(since0.is_multiple_of(0)returns true). This causes a division-by-zero panic at line 117 and line 327 instead of a clear assertion failure.Add
assert!(num_heads > 0, ...)before eachis_multiple_ofcheck:Suggested patch
pub fn new(d_model: usize, num_heads: usize) -> Self { + assert!(num_heads > 0, "num_heads ({}) must be > 0", num_heads); assert!( d_model.is_multiple_of(num_heads), "d_model ({}) must be divisible by num_heads ({})", d_model, num_heads );pub fn new(d_query: usize, d_kv: usize, d_model: usize, num_heads: usize) -> Self { + assert!(num_heads > 0, "num_heads ({}) must be > 0", num_heads); assert!( d_model.is_multiple_of(num_heads), "d_model must be divisible by num_heads" );Also applies to: 275-278
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/networks/attention.rs` around lines 50 - 55, Add an explicit guard asserting num_heads > 0 before the existing is_multiple_of check in both SelfAttention::new and CrossAttention::new so a zero-heads case fails with a clear assertion instead of causing a later division-by-zero; specifically, insert assert!(num_heads > 0, "num_heads ({}) must be > 0", num_heads) immediately before the assert!(d_model.is_multiple_of(num_heads), ...) in the SelfAttention::new and CrossAttention::new constructors so the invalid input is caught early and prevents downstream panics.
🧹 Nitpick comments (3)
src/checkpoint.rs (1)
910-923: Consider adding explicitNaN/INFINITYcases to lock down normalization semantics.Current coverage validates
NEG_INFINITY; addingNaNandINFINITYinputs would make the non-finite-to-NEG_INFINITYcontract unambiguous and prevent accidental drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/checkpoint.rs` around lines 910 - 923, Extend the existing test_checkpoint_best_reward_non_finite_roundtrip test (or add two small tests) to cover f32::NAN and f32::INFINITY inputs: create a Checkpoint (use Checkpoint::new()) set timesteps and best_reward to f32::NAN and separately to f32::INFINITY, save via CheckpointManager::save and reload via CheckpointManager::load, and assert that loaded.best_reward equals f32::NEG_INFINITY (matching the non-finite normalization contract). Reference CheckpointManager, Checkpoint, and the test_checkpoint_best_reward_non_finite_roundtrip test for where to add these cases so the semantics are locked down.src/lib.rs (1)
19-27: Consider more targeted lint suppression.Crate-wide
#![allow(...)]can mask legitimate issues in new code. Prefer module or function-level#[allow(...)]attributes where the lint is actually triggered. This preserves Clippy's ability to catch real problems elsewhere.For example,
clippy::only_used_in_recursionandclippy::if_same_then_elsecan identify actual bugs—silencing them globally reduces their value as safety nets.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib.rs` around lines 19 - 27, The crate currently uses a global #![allow(...)] in src/lib.rs which mutes many Clippy lints; change this to targeted, narrower attributes by removing or reducing the crate-level #![allow(...)] and instead apply #[allow(...)] on the specific modules or functions that actually trigger each lint (e.g., move clippy::only_used_in_recursion and clippy::if_same_then_else to the precise module or fn where they occur), keep only truly unavoidable global allows, and run clippy to iteratively place the remaining allows on the exact items (search for the attribute block `#![allow(...)]` and replace with per-module or per-function `#[allow(...)]` annotations)..github/workflows/release.yml (1)
27-34: Migrate from archived GitHub Actions to reduce CI resilience risks.The workflow uses archived, unmaintained actions:
actions/create-release@v1andactions/upload-release-asset@v1. These will eventually break. Recommended alternatives:
- softprops/action-gh-release (v2.6.1, actively maintained) — handles both release creation and asset uploads
- GitHub CLI (
gh release create) — official recommendation per GitHub DocsConsider upgrading to one of these to ensure long-term stability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 27 - 34, The workflow currently uses the archived actions actions/create-release@v1 and actions/upload-release-asset@v1 in the job with id create_release; replace these with a maintained alternative (e.g., softprops/action-gh-release@v2.6.1) or use the GitHub CLI (gh release create) so release creation and asset upload are handled by a supported tool. Update the step that uses actions/create-release@v1 to call softprops/action-gh-release (or a run: gh release create sequence), map tag_name and release_name inputs to the new action's inputs or CLI args, and consolidate any separate upload steps (previously using upload-release-asset@v1) to the new action/CLI to ensure assets are attached correctly. Ensure GITHUB_TOKEN or GH_TOKEN is provided and adjust step names (create_release) and outputs (steps.get_version.outputs.version) usage to match the new action/CLI input names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benches/env_benchmark.rs`:
- Line 8: The import list removed VecEnv but the type VecEnv is still referenced
(e.g., in the benchmark setup), causing builds with the distributed feature to
fail; re-add VecEnv to the imports alongside Environment, MarketData, and
TradingEnv (i.e., include VecEnv in the use octane_rs::envs::{...} clause) so
distributed builds can find the symbol and compile.
In `@QUICKSTART.md`:
- Around line 23-28: The quickstart snippet incorrectly calls
MarketData::random(1000); replace that call with the correct API
MarketData::synthetic which requires both a timestep count and a random seed
(e.g., use timesteps=1000 and seed=42). Locate the MarketData invocation (the
line calling MarketData::random) and update it to call MarketData::synthetic
with the two required arguments so the code compiles against the current API.
In `@src/algorithms/ppo.rs`:
- Around line 151-172: The continuous-action path currently emits raw
means/samples which can exceed environment bounds; update the policy
action-sampling/forward path (the code that uses is_discrete, the policy outputs
from init_networks and log_std) to clamp any continuous actions to the BoxSpace
bounds before returning them to the environment. Locate where actions are
produced for non-discrete spaces (use symbols is_discrete, act_space, the policy
network outputs/mean and log_std) and apply clamping using act_space.low and
act_space.high (or the environment's clip method) to ensure returned actions lie
within the BoxSpace limits.
- Around line 107-108: The checkpoint currently only serializes model weights so
optimizer moments are lost; update the PPO checkpoint save/load to include the
optimizer state for the optimizer field (optimizer: AdamW). Modify the save() to
serialize the optimizer's full state (moment estimates, step counts,
hyperparameters) alongside model weights and modify load() to restore that state
into a freshly constructed AdamW optimizer instance so resumed training
continues with the same moments; also add compatibility/version metadata to
handle missing optimizer state gracefully when loading older checkpoints.
In `@src/envs/space.rs`:
- Around line 13-16: The trait method is_continuous currently has a default impl
returning false which can silently misclassify custom continuous spaces; remove
the default implementation in the Space trait (declare fn is_continuous(&self)
-> bool; with no body) so implementations must explicitly provide it, then
update any types implementing Space to add an is_continuous method (implement
true for continuous spaces and false for discrete ones) and run the build to fix
compile errors introduced by this API tightening.
In `@src/simd/mod.rs`:
- Around line 41-42: The attribute declaration #![allow(missing_docs)] is
currently inside the //! ```ignore doc example and thus only part of the docs;
remove it from the code example and relocate it to be an actual module attribute
(either move it to the top of the file before the doc comment or place it after
the doc block alongside the existing #![allow(unsafe_code)]), ensuring the
attribute applies to the module rather than appearing in the example text.
In `@src/strategies/meta.rs`:
- Around line 798-799: MetaLearningConfig currently allows
regime_detection_frequency == 0 which makes is_multiple_of behave incorrectly;
update MetaLearningConfig::validate() to reject zero by adding a check that
regime_detection_frequency > 0 and return/propagate a validation error (matching
the pattern used for TD3Config::policy_delay validation) so callers cannot
construct a config with zero frequency; reference the field
regime_detection_frequency and the validate() method in MetaLearningConfig when
adding this explicit check and appropriate error message.
---
Outside diff comments:
In `@benches/gpu_benchmark.rs`:
- Around line 9-18: The vector `devices` is declared immutable but later
mutated; change its declaration to be mutable (e.g., `let mut devices =
vec![("CPU", candle_core::Device::Cpu)];`) so that the subsequent
`devices.push(("Metal", metal))` inside the `#[cfg(feature = "metal")]` block
can compile; locate the `devices` binding and the call to
`candle_core::Device::new_metal` to apply this fix.
In `@src/buffer/nstep.rs`:
- Around line 244-252: When encountered_done is true the code currently picks
the done transition’s obs from self.n_step_buffer as n_step_next_obs; instead
use the terminal successor final_next_obs instead. Update the encountered_done
branch (where n_step_next_obs and n_step_done are set) to assign n_step_next_obs
= final_next_obs.to_vec() (with the same unwrap fallback if needed) and keep
n_step_done = true; reference symbols: encountered_done, n_step_next_obs,
n_step_done, n_step_buffer, actual_n, final_next_obs.
In `@src/networks/attention.rs`:
- Around line 50-55: Add an explicit guard asserting num_heads > 0 before the
existing is_multiple_of check in both SelfAttention::new and CrossAttention::new
so a zero-heads case fails with a clear assertion instead of causing a later
division-by-zero; specifically, insert assert!(num_heads > 0, "num_heads ({})
must be > 0", num_heads) immediately before the
assert!(d_model.is_multiple_of(num_heads), ...) in the SelfAttention::new and
CrossAttention::new constructors so the invalid input is caught early and
prevents downstream panics.
In `@src/simd/mod.rs`:
- Around line 382-386: The alignment check uses usize::is_multiple_of
(stabilized in Rust 1.73) which may break on older toolchains; either declare
MSRV by adding rust-version = "1.73" to Cargo.toml or change the check in the
function that uses addr and NEON_ALIGNMENT (the block returning
SimdError::AlignmentError) to use a portable expression: test with addr %
NEON_ALIGNMENT == 0 (and keep actual: addr % NEON_ALIGNMENT) instead of
is_multiple_of to preserve behavior on older Rust versions.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 27-34: The workflow currently uses the archived actions
actions/create-release@v1 and actions/upload-release-asset@v1 in the job with id
create_release; replace these with a maintained alternative (e.g.,
softprops/action-gh-release@v2.6.1) or use the GitHub CLI (gh release create) so
release creation and asset upload are handled by a supported tool. Update the
step that uses actions/create-release@v1 to call softprops/action-gh-release (or
a run: gh release create sequence), map tag_name and release_name inputs to the
new action's inputs or CLI args, and consolidate any separate upload steps
(previously using upload-release-asset@v1) to the new action/CLI to ensure
assets are attached correctly. Ensure GITHUB_TOKEN or GH_TOKEN is provided and
adjust step names (create_release) and outputs
(steps.get_version.outputs.version) usage to match the new action/CLI input
names.
In `@src/checkpoint.rs`:
- Around line 910-923: Extend the existing
test_checkpoint_best_reward_non_finite_roundtrip test (or add two small tests)
to cover f32::NAN and f32::INFINITY inputs: create a Checkpoint (use
Checkpoint::new()) set timesteps and best_reward to f32::NAN and separately to
f32::INFINITY, save via CheckpointManager::save and reload via
CheckpointManager::load, and assert that loaded.best_reward equals
f32::NEG_INFINITY (matching the non-finite normalization contract). Reference
CheckpointManager, Checkpoint, and the
test_checkpoint_best_reward_non_finite_roundtrip test for where to add these
cases so the semantics are locked down.
In `@src/lib.rs`:
- Around line 19-27: The crate currently uses a global #![allow(...)] in
src/lib.rs which mutes many Clippy lints; change this to targeted, narrower
attributes by removing or reducing the crate-level #![allow(...)] and instead
apply #[allow(...)] on the specific modules or functions that actually trigger
each lint (e.g., move clippy::only_used_in_recursion and
clippy::if_same_then_else to the precise module or fn where they occur), keep
only truly unavoidable global allows, and run clippy to iteratively place the
remaining allows on the exact items (search for the attribute block
`#![allow(...)]` and replace with per-module or per-function `#[allow(...)]`
annotations).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63ad1ac6-fadd-4476-b142-4a1651846365
📒 Files selected for processing (45)
.github/workflows/release.ymlCargo.tomlQUICKSTART.mdREADME.mdbenches/env_benchmark.rsbenches/gpu_benchmark.rsexamples/benchmark_vs_sb3.rsexamples/trading_metrics_demo.rssrc/algorithms/config.rssrc/algorithms/cql.rssrc/algorithms/dqn.rssrc/algorithms/iqn.rssrc/algorithms/ppo.rssrc/algorithms/rollout.rssrc/algorithms/td3.rssrc/backtesting/walk_forward.rssrc/bin/octane-tui.rssrc/buffer/her.rssrc/buffer/nstep.rssrc/checkpoint.rssrc/core/precision.rssrc/distributions/gaussian.rssrc/envs/space.rssrc/lib.rssrc/logging/mod.rssrc/logging/tensorboard.rssrc/logging/wandb.rssrc/metrics/attribution.rssrc/metrics/journal.rssrc/networks/attention.rssrc/networks/transformer.rssrc/risk/drawdown.rssrc/risk/position_sizing.rssrc/simd/gae.rssrc/simd/log_prob.rssrc/simd/mod.rssrc/simd/td_error.rssrc/strategies/ensemble.rssrc/strategies/imitation.rssrc/strategies/meta.rssrc/trading/env.rssrc/trading/multi_timeframe.rssrc/trading/regime.rssrc/tui/theme.rssrc/tuning.rs
💤 Files with no reviewable changes (1)
- src/bin/octane-tui.rs
| use criterion::{black_box, criterion_group, criterion_main, BenchmarkId, Criterion}; | ||
| use octane_rs::core::Device; | ||
| use octane_rs::envs::{Environment, MarketData, Space, TradingEnv, VecEnv}; | ||
| use octane_rs::envs::{Environment, MarketData, TradingEnv}; |
There was a problem hiding this comment.
Re-add VecEnv import for distributed builds
VecEnv is still referenced at Line 62, but it was removed from imports at Line 8. This breaks compilation when distributed is enabled.
💡 Suggested fix
use octane_rs::core::Device;
use octane_rs::envs::{Environment, MarketData, TradingEnv};
+#[cfg(feature = "distributed")]
+use octane_rs::envs::VecEnv;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use octane_rs::envs::{Environment, MarketData, TradingEnv}; | |
| use octane_rs::envs::{Environment, MarketData, TradingEnv}; | |
| #[cfg(feature = "distributed")] | |
| use octane_rs::envs::VecEnv; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benches/env_benchmark.rs` at line 8, The import list removed VecEnv but the
type VecEnv is still referenced (e.g., in the benchmark setup), causing builds
with the distributed feature to fail; re-add VecEnv to the imports alongside
Environment, MarketData, and TradingEnv (i.e., include VecEnv in the use
octane_rs::envs::{...} clause) so distributed builds can find the symbol and
compile.
| use octane_rs::prelude::*; | ||
| use octane_rs::envs::{TradingEnv, MarketData}; | ||
| use octane_rs::algorithms::{PPOConfig, PPOAgent, RLAlgorithm}; | ||
| use octane_rs::core::Device; | ||
|
|
||
| fn main() -> rocket_rs::Result<()> { | ||
| fn main() -> octane_rs::Result<()> { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify symbol presence used in QUICKSTART
rg -nP --type=rust -C3 '\b(pub\s+)?(struct|type)\s+MarketData\b'
rg -nP --type=rust -C3 '\b(pub\s+)?struct\s+TradingEnv\b'
rg -nP --type=rust -C5 '\bimpl\s+TradingEnv\b'
rg -nP --type=rust -C5 '\bimpl\s+PPOAgent\b'
# Inspect constructor signatures likely referenced by QUICKSTART
rg -nP --type=rust -C3 '\bfn\s+new\s*\([^)]*MarketData[^)]*\)'
rg -nP --type=rust -C3 '\bfn\s+new\s*\([^)]*PPOConfig[^)]*\)'Repository: lubluniky/rocket-rs
Length of output: 1277
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for PPOAgent structure definition
echo "=== Searching for PPOAgent ==="
rg -nP --type=rust '\b(pub\s+)?struct\s+PPOAgent\b' -C3 || true
# Search for PPOConfig structure definition
echo "=== Searching for PPOConfig ==="
rg -nP --type=rust '\b(pub\s+)?struct\s+PPOConfig\b' -C3 || true
# Check for impl blocks for PPOAgent
echo "=== Searching for PPOAgent impl blocks ==="
rg -nP --type=rust '\bimpl\s+PPOAgent\b' -C3 || true
# Check for new methods in algorithms module
echo "=== Searching for PPOAgent::new ==="
rg -nP --type=rust 'fn\s+new\s*\(' src/algorithms/ -C2 || true
# List the main lib.rs or mod.rs to understand module structure
echo "=== Checking module structure ==="
find . -name "lib.rs" -o -name "main.rs" | head -5 | xargs -I {} cat -n {}Repository: lubluniky/rocket-rs
Length of output: 23558
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for RLAlgorithm definition
echo "=== Searching for RLAlgorithm ==="
rg -nP --type=rust '\b(pub\s+)?(trait|struct|type)\s+RLAlgorithm\b' || true
# Check if it's mentioned anywhere
rg -nP --type=rust 'RLAlgorithm' || true
# Also check the full QUICKSTART.md file
echo "=== Full QUICKSTART.md content ==="
wc -l QUICKSTART.md
cat QUICKSTART.mdRepository: lubluniky/rocket-rs
Length of output: 6121
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for MarketData::random method
echo "=== Searching for MarketData::random ==="
rg -nP --type=rust 'impl.*MarketData' -A 30 | head -50 || true
# Search for VecEnv definition and new method
echo "=== Searching for VecEnv definition ==="
rg -nP --type=rust '\b(pub\s+)?struct\s+VecEnv\b' -A 5 || true
echo "=== Searching for VecEnv::new ==="
rg -nP --type=rust 'fn\s+new.*VecEnv' -B 2 -A 5 || true
# Check if VecEnv is in prelude
echo "=== Checking VecEnv in prelude (lib.rs) ==="
grep -n "VecEnv" src/lib.rs || trueRepository: lubluniky/rocket-rs
Length of output: 11722
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the full VecEnv impl block
echo "=== Full VecEnv impl block ==="
rg -nP --type=rust 'impl.*VecEnv' -A 100 | head -150 || true
# Search for any constructors in vecenv.rs file
echo "=== All public methods in vecenv.rs ==="
rg -nP --type=rust 'pub fn' src/envs/vecenv.rs | head -20 || true
# Check if MarketData has any impl block
echo "=== MarketData impl block ==="
rg -nP --type=rust 'impl.*MarketData' -A 50 src/envs/trading.rs | head -100 || trueRepository: lubluniky/rocket-rs
Length of output: 11565
Fix MarketData constructor call in quickstart code snippet.
The QUICKSTART uses MarketData::random(1000) which does not exist. The correct method is MarketData::synthetic(timesteps, seed) which requires both a timestep count and a random seed. Update line 28 to use the correct API:
let data = MarketData::synthetic(1000, 42);All other imports and API signatures (TradingEnv::new(), PPOAgent::new(), PPOConfig, VecEnv::new(), RLAlgorithm) are valid and match the current codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@QUICKSTART.md` around lines 23 - 28, The quickstart snippet incorrectly calls
MarketData::random(1000); replace that call with the correct API
MarketData::synthetic which requires both a timestep count and a random seed
(e.g., use timesteps=1000 and seed=42). Locate the MarketData invocation (the
line calling MarketData::random) and update it to call MarketData::synthetic
with the two required arguments so the code compiles against the current API.
| /// Optimizer for policy and value networks. | ||
| optimizer: AdamW, |
There was a problem hiding this comment.
Checkpoint resumability is still incomplete.
The optimizer now persists across updates in memory, but save() / load() still only serialize model weights. Reloading into a fresh agent will reset optimizer moments, so resumed training will diverge from an uninterrupted run and miss the checkpoint objective from this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/algorithms/ppo.rs` around lines 107 - 108, The checkpoint currently only
serializes model weights so optimizer moments are lost; update the PPO
checkpoint save/load to include the optimizer state for the optimizer field
(optimizer: AdamW). Modify the save() to serialize the optimizer's full state
(moment estimates, step counts, hyperparameters) alongside model weights and
modify load() to restore that state into a freshly constructed AdamW optimizer
instance so resumed training continues with the same moments; also add
compatibility/version metadata to handle missing optimizer state gracefully when
loading older checkpoints.
| let is_discrete = !act_space.is_continuous(); | ||
| let hidden_sizes = config.hidden_sizes.clone(); | ||
|
|
||
| let rng = match config.seed { | ||
| Some(seed) => StdRng::seed_from_u64(seed), | ||
| None => StdRng::from_entropy(), | ||
| }; | ||
|
|
||
| let var_map = VarMap::new(); | ||
| let hidden_sizes = vec![64, 64]; // Default MLP architecture | ||
| let policy_prefix = "policy".to_string(); | ||
| let value_prefix = "value".to_string(); | ||
| let current_lr = config.learning_rate; | ||
| let log_std = init_networks( | ||
| &var_map, | ||
| device, | ||
| obs_dim, | ||
| act_dim, | ||
| &hidden_sizes, | ||
| &policy_prefix, | ||
| &value_prefix, | ||
| is_discrete, | ||
| )?; |
There was a problem hiding this comment.
Continuous actions are still returned out of bounds.
This change correctly routes 1-D continuous spaces through the continuous policy path, but that path still returns raw means/samples. For bounded BoxSpace environments, that can now surface as invalid actions being sent to the env. Please clamp continuous outputs to the action-space bounds before returning them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/algorithms/ppo.rs` around lines 151 - 172, The continuous-action path
currently emits raw means/samples which can exceed environment bounds; update
the policy action-sampling/forward path (the code that uses is_discrete, the
policy outputs from init_networks and log_std) to clamp any continuous actions
to the BoxSpace bounds before returning them to the environment. Locate where
actions are produced for non-discrete spaces (use symbols is_discrete,
act_space, the policy network outputs/mean and log_std) and apply clamping using
act_space.low and act_space.high (or the environment's clip method) to ensure
returned actions lie within the BoxSpace limits.
| /// Whether this space represents continuous values. | ||
| fn is_continuous(&self) -> bool { | ||
| false | ||
| } |
There was a problem hiding this comment.
is_continuous defaulting to false can silently misclassify custom continuous spaces.
Any external Space implementer that forgets to override this new method now compiles but gets treated as discrete by PPO. Consider making this method required (no default body) to fail fast at compile time.
Suggested API tightening
pub trait Space: Clone + Send + Sync {
/// Shape of a single sample from this space.
fn shape(&self) -> &[usize];
/// Whether this space represents continuous values.
- fn is_continuous(&self) -> bool {
- false
- }
+ fn is_continuous(&self) -> bool;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Whether this space represents continuous values. | |
| fn is_continuous(&self) -> bool { | |
| false | |
| } | |
| /// Whether this space represents continuous values. | |
| fn is_continuous(&self) -> bool; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/envs/space.rs` around lines 13 - 16, The trait method is_continuous
currently has a default impl returning false which can silently misclassify
custom continuous spaces; remove the default implementation in the Space trait
(declare fn is_continuous(&self) -> bool; with no body) so implementations must
explicitly provide it, then update any types implementing Space to add an
is_continuous method (implement true for continuous spaces and false for
discrete ones) and run the build to fix compile errors introduced by this API
tightening.
| #![allow(missing_docs)] | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the module compiles correctly
cargo check -p octane-rs --lib 2>&1 | grep -A5 "simd/mod.rs" | head -20Repository: lubluniky/rocket-rs
Length of output: 45
🏁 Script executed:
head -60 src/simd/mod.rs | cat -nRepository: lubluniky/rocket-rs
Length of output: 2676
Remove #![allow(missing_docs)] from inside the doc comment example block.
The attribute on line 41 is embedded within the //! ```ignore code example (lines 37-54) and is treated as documentation text rather than an actual module attribute. It has no effect on the module. Move it to the file start before the doc comment, or place it after the doc comment block near line 56 where #![allow(unsafe_code)] is properly positioned.
Suggested fix
//! SIMD-accelerated operations for Octane
+#![allow(missing_docs)]
//!
//! This module provides high-performance SIMD operations optimized for multipleOr after line 54, before line 56.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/simd/mod.rs` around lines 41 - 42, The attribute declaration
#![allow(missing_docs)] is currently inside the //! ```ignore doc example and
thus only part of the docs; remove it from the code example and relocate it to
be an actual module attribute (either move it to the top of the file before the
doc comment or place it after the doc block alongside the existing
#![allow(unsafe_code)]), ensuring the attribute applies to the module rather
than appearing in the example text.
| let (regime, _confidence) = if self.total_timesteps.is_multiple_of(self.config.regime_detection_frequency) { | ||
| let (r, c) = self.detect_regime(&context_embedding)?; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the MetaLearningConfig definition and validation
fd -t f "meta\.rs" | head -20Repository: lubluniky/rocket-rs
Length of output: 85
🏁 Script executed:
# Search for MetaLearningConfig struct and any validate method
rg "struct MetaLearningConfig|impl.*MetaLearningConfig.*validate" --type rustRepository: lubluniky/rocket-rs
Length of output: 117
🏁 Script executed:
# Read the meta.rs file to understand the context around lines 798-799
wc -l src/strategies/meta.rsRepository: lubluniky/rocket-rs
Length of output: 90
🏁 Script executed:
# Find where MetaLearningConfig is defined
rg "pub struct MetaLearningConfig" -A 30 src/strategies/meta.rsRepository: lubluniky/rocket-rs
Length of output: 1145
🏁 Script executed:
# Check for any validate method in the file
rg "fn validate" src/strategies/meta.rs -B 2 -A 10Repository: lubluniky/rocket-rs
Length of output: 563
🏁 Script executed:
# Read lines 795-805 to see the context around the issue
sed -n '795,805p' src/strategies/meta.rsRepository: lubluniky/rocket-rs
Length of output: 522
🏁 Script executed:
# Get the complete validate() method
rg "fn validate" src/strategies/meta.rs -A 30Repository: lubluniky/rocket-rs
Length of output: 1074
🏁 Script executed:
# Verify Rust's is_multiple_of behavior by checking documentation or examples
# Let's also look for any existing checks on regime_detection_frequency
rg "regime_detection_frequency" src/strategies/meta.rsRepository: lubluniky/rocket-rs
Length of output: 218
🏁 Script executed:
# Let me verify the exact behavior of is_multiple_of(0) by checking Rust documentation
# or creating a simple test. First, let's check if there are any tests in the repo
rg "is_multiple_of" -A 2 -B 2 src/Repository: lubluniky/rocket-rs
Length of output: 4654
🏁 Script executed:
# Let's also check the full context of lines around 798-799 to understand the implications
sed -n '790,810p' src/strategies/meta.rsRepository: lubluniky/rocket-rs
Length of output: 704
🏁 Script executed:
# Let me verify the behavior of is_multiple_of(0) by checking if there's any documentation or tests
# First, let's search for how similar frequency/delay configs are validated elsewhere
rg "fn validate" src/algorithms/ -A 20 | grep -A 20 "policy_delay\|train_freq"Repository: lubluniky/rocket-rs
Length of output: 1322
🏁 Script executed:
# Check if there are any constraints on these configs in other places
rg "config\.policy_delay|config\.train_freq|config\.auto_flush_interval" src/ -B 2 -A 2Repository: lubluniky/rocket-rs
Length of output: 2150
🏁 Script executed:
# Let's verify the exact behavior by checking Rust documentation or any tests that might exist
# Search for any tests involving is_multiple_of or regime_detection_frequency
rg "regime_detection_frequency|is_multiple_of" src/ -l | xargs grep -l "test\|#\[test\]"Repository: lubluniky/rocket-rs
Length of output: 334
🏁 Script executed:
# Check if there's any documentation or comments explaining is_multiple_of behavior
rg "is_multiple_of" --type rust -B 3 -A 3 | grep -E "panic|zero|divisor" | head -20Repository: lubluniky/rocket-rs
Length of output: 45
🌐 Web query:
Rust usize is_multiple_of behavior with zero divisor
💡 Result:
Rust's usize type does not have a method named is_multiple_of in the standard library as of the latest stable documentation. The official Rust std::primitive.usize docs do not list an is_multiple_of method. Instead, std provides checked_next_multiple_of which explicitly returns None if rhs is zero, and division operations like / and % panic on zero divisor. The is_multiple_of method appears in the external num crate (num::Integer trait), implemented for usize. The docs describe it as returning true if self is a multiple of other, with examples like 9.is_multiple_of(&3) == true, but no explicit zero divisor example. Given typical implementations (self % other == 0 when other != 0), it likely panics on zero divisor like std % operator, as no special zero handling is documented. Recently (merged July 2024, stabilized in Rust 1.82+ per tracking issue), std added is_multiple_of to unsigned primitives including usize behind feature(unsigned_is_multiple_of), but as of 2026-04-16 it may be stable. This new method handles zero explicitly: 0.is_multiple_of(0) == true, n.is_multiple_of(0) == false for n != 0, avoiding panic. Without specifying std or num, or Rust version, the common prior behavior (panic on zero) applies to both % and likely num::is_multiple_of. Use checked_rem or if-checks for safe multiple checking with zero.
Citations:
- 1: https://doc.rust-lang.org/std/primitive.usize.html
- 2: https://docs.rust-lang.org/std/primitive.usize.html
- 3: https://doc.rust-lang.org/stable/std/primitive.usize.html
- 4: https://docs.rs/num/latest/num/trait.Integer.html
- 5: Tracking Issue for
unsigned_is_multiple_ofrust-lang/rust#128101 - 6: add
is_multiple_offor unsigned integer types rust-lang/rust#128103
Add regime_detection_frequency validation to reject zero values.
usize::is_multiple_of(0) returns false for all non-zero values and true only at zero (Rust 1.82+). With regime_detection_frequency == 0, regime detection would only run once at startup (when total_timesteps == 0), then effectively be disabled without error. Add validation to MetaLearningConfig::validate() to require regime_detection_frequency > 0, consistent with similar frequency/delay parameters like TD3Config::policy_delay.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/strategies/meta.rs` around lines 798 - 799, MetaLearningConfig currently
allows regime_detection_frequency == 0 which makes is_multiple_of behave
incorrectly; update MetaLearningConfig::validate() to reject zero by adding a
check that regime_detection_frequency > 0 and return/propagate a validation
error (matching the pattern used for TD3Config::policy_delay validation) so
callers cannot construct a config with zero frequency; reference the field
regime_detection_frequency and the validate() method in MetaLearningConfig when
adding this explicit check and appropriate error message.
1 participant
Summary
octane-tuicanonical binary, release artifact naming, metadata/docs alignment tooctane-rs)cargo packagepassingTDD / Regression Coverage
src/algorithms/ppo.rssrc/algorithms/rollout.rssrc/simd/gae.rssrc/buffer/nstep.rssrc/checkpoint.rssrc/core/precision.rssrc/envs/space.rsVerification
cargo test --libcargo test --examplescargo test --doccargo clippy --all-targets -- -D warningscargo build --releasecargo build --release --features metal,simdcargo test --lib --features metal,simdcargo bench env_benchmarkcargo bench ppo_benchmarkcargo bench gpu_benchmarkcargo package --allow-dirtyNotes
xcrun -sdk macosx metal -vis available in this environment (M4).Closes #23
Closes #24
Summary by CodeRabbit
Release Notes
Breaking Changes
New Features
hidden_sizesconfiguration option.Bug Fixes
Improvements