Add Prophet WASM example #152
Conversation
|
Warning Rate limit exceeded@sd2k has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 17 seconds before requesting another review. ⌛ 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. WalkthroughThe pull request introduces several modifications to the Rust project, primarily focusing on the workflow configuration and package dependencies. The Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
crates/augurs-prophet/Cargo.toml (1)
52-56: LGTM! Consider adding a documentation link.The feature rename and updated comments accurately reflect its broader scope covering both cmdstan and wasmstan build failures. The warning about potential build breakage is clear and helpful.
Consider adding a link to the project's documentation about build configuration to help future maintainers understand the complete context:
# Ignore cmdstan compilation or wasmstan copying in the build script. # This should only be used for developing the library, not by # end users, or you may end up with a broken build where the # Prophet model isn't available to be compiled into the binary. +# See: <link-to-build-configuration-docs> internal-ignore-build-failures = []examples/forecasting/examples/prophet_wasmstan.rs (2)
44-49: Consider making the model path configurable.The hardcoded path to the Prophet model binary could cause issues in different environments. Consider using an environment variable or configuration file.
Apply this diff to make the path configurable:
- let cmdstan = CmdstanOptimizer::with_prophet_path("prophet_stan_model/prophet_model.bin")? + let model_path = std::env::var("PROPHET_MODEL_PATH") + .unwrap_or_else(|_| "prophet_stan_model/prophet_model.bin".to_string()); + let cmdstan = CmdstanOptimizer::with_prophet_path(&model_path)?
54-60: Enhance example with error cases and validation.The example could be more comprehensive by:
- Demonstrating how to handle prediction errors
- Showing model validation techniques
- Providing examples of different regressor configurations
Would you like me to provide additional example code demonstrating these scenarios?
crates/augurs-prophet/build.rs (2)
91-106: LGTM! Consider enhancing documentation.The error handling logic for CmdStan builds is well-structured. The feature rename from
internal-ignore-cmdstan-failuretointernal-ignore-build-failuresmakes sense as it now covers both build types.Consider adding a warning comment that explicitly states this is for development/CI only:
// This is a complete hack but lets us get away with still using // the `--all-features` flag of Cargo without everything failing // if there isn't a Stan installation. + // WARNING: This feature is intended for development/CI only and should never + // be enabled in production builds as it will cause runtime failures.
132-154: Consider refactoring to reduce code duplication.While the WASMStan error handling implementation is correct, there's significant duplication with the CmdStan error handling logic. Consider extracting the common error handling pattern into a shared function.
Here's a suggested refactoring:
fn handle_build_failure( result: Result<(), Box<dyn std::error::Error>>, empty_files: &[&str], ) -> Result<(), Box<dyn std::error::Error>> { #[cfg(feature = "internal-ignore-build-failures")] if result.is_err() { create_empty_files(empty_files)?; return Ok(()); } #[cfg(not(feature = "internal-ignore-build-failures"))] if std::env::var("DOCS_RS").is_ok() { create_empty_files(empty_files)?; return Ok(()); } #[cfg(not(feature = "internal-ignore-build-failures"))] if std::env::var("DOCS_RS").is_err() { result?; } Ok(()) } // Usage in handle_cmdstan: handle_build_failure(_result, &["prophet", "libtbb.so.12"])?; // Usage in handle_wasmstan: handle_build_failure(_result, &["prophet-wasmstan.wasm"])?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
.github/workflows/rust.yml(2 hunks)crates/augurs-prophet/Cargo.toml(1 hunks)crates/augurs-prophet/build.rs(2 hunks)examples/forecasting/Cargo.toml(1 hunks)examples/forecasting/examples/prophet_wasmstan.rs(1 hunks)
🔇 Additional comments (6)
examples/forecasting/Cargo.toml (2)
14-22: LGTM! Clean formatting and proper feature addition.
The multi-line formatting of features improves readability and follows Rust community best practices. The addition of "prophet-wasmstan" feature aligns well with the PR objective.
14-22: Verify feature compatibility.
Since both "prophet-cmdstan" and "prophet-wasmstan" features are enabled, let's verify they are designed to work together.
.github/workflows/rust.yml (3)
5-5: LGTM!
The formatting change improves readability while maintaining functionality.
42-42: LGTM!
The comment update provides clearer context about the purpose of downloading the Prophet Stan model.
46-52: Verify artifact availability and consider adding error handling.
The new step to download the Prophet WASMStan component is correctly positioned and well-documented. However, we should verify:
- That the artifact is always available when needed
- That the workflow handles potential download failures gracefully
Consider these improvements:
- Add
if: success()condition to ensure this step only runs if previous steps succeeded - Add error handling in the subsequent steps that depend on this artifact
- name: Download Prophet WASMStan component artifact
uses: actions/download-artifact@v4
+ if: success()
with:
name: prophet-wasmstan.wasm
path: crates/augurs-prophet
+ continue-on-error: trueLet's verify the artifact availability:
crates/augurs-prophet/build.rs (1)
140-143: Verify feature flag configuration.
Let's ensure the internal-ignore-build-failures feature is properly configured in Cargo.toml.
✅ Verification successful
Feature flag is properly configured in Cargo.toml
The internal-ignore-build-failures feature is correctly defined in crates/augurs-prophet/Cargo.toml as an empty feature flag []. It's only used in this crate and not referenced by any other crates in the repository, which is appropriate for an internal build-specific feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check feature configuration in Cargo.toml
# Test: Verify the feature is defined in Cargo.toml
rg -A 5 'internal-ignore-build-failures' crates/augurs-prophet/Cargo.toml
# Test: Check if any other crates depend on this feature
rg 'internal-ignore-build-failures' -g 'Cargo.toml'
Length of output: 260
| //! $ cargo run --features download --bin download-stan-model | ||
| //! $ cargo run --example prophet_cmdstan | ||
| //! ``` |
There was a problem hiding this comment.
Fix inconsistent example name in documentation.
The command shows cargo run --example prophet_cmdstan but the file is named prophet_wasmstan.rs. This mismatch could confuse users trying to run the example.
Apply this diff to fix the documentation:
- $ cargo run --example prophet_cmdstan
+ $ cargo run --example prophet_wasmstan📝 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.
| //! $ cargo run --features download --bin download-stan-model | |
| //! $ cargo run --example prophet_cmdstan | |
| //! ``` | |
| //! $ cargo run --features download --bin download-stan-model | |
| //! $ cargo run --example prophet_wasmstan | |
| //! ``` |
| let ds = vec![ | ||
| 1704067200, 1704871384, 1705675569, 1706479753, 1707283938, 1708088123, 1708892307, | ||
| 1709696492, 1710500676, 1711304861, 1712109046, 1712913230, 1713717415, | ||
| ]; | ||
| let y = vec![ | ||
| 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0, 11.0, 12.0, 13.0, | ||
| ]; | ||
| let data = TrainingData::new(ds, y.clone())? | ||
| .with_regressors(HashMap::from([ | ||
| ( | ||
| "foo".to_string(), | ||
| vec![ | ||
| 1.0, 2.0, 1.0, 2.0, 1.0, 2.0, 1.0, 2.0, 1.0, 2.0, 1.0, 2.0, 1.0, | ||
| ], | ||
| ), | ||
| ( | ||
| "bar".to_string(), | ||
| vec![ | ||
| 4.0, 5.0, 6.0, 4.0, 5.0, 6.0, 4.0, 5.0, 6.0, 4.0, 5.0, 6.0, 4.0, | ||
| ], | ||
| ), | ||
| ])) | ||
| .unwrap(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and data documentation.
- The
unwrap()on line 42 could cause runtime panics. Consider using the?operator for consistent error handling. - The test data could benefit from documentation explaining what the timestamps and values represent.
Apply this diff to improve error handling and documentation:
let ds = vec![
1704067200, 1704871384, 1705675569, 1706479753, 1707283938, 1708088123, 1708892307,
1709696492, 1710500676, 1711304861, 1712109046, 1712913230, 1713717415,
- ];
+ ]; // Unix timestamps starting from 2024-01-01
let y = vec![
1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0, 11.0, 12.0, 13.0,
- ];
+ ]; // Example values with linear growth
let data = TrainingData::new(ds, y.clone())?
.with_regressors(HashMap::from([
(
"foo".to_string(),
vec![
1.0, 2.0, 1.0, 2.0, 1.0, 2.0, 1.0, 2.0, 1.0, 2.0, 1.0, 2.0, 1.0,
],
),
(
"bar".to_string(),
vec![
4.0, 5.0, 6.0, 4.0, 5.0, 6.0, 4.0, 5.0, 6.0, 4.0, 5.0, 6.0, 4.0,
],
),
- ]))
- .unwrap();
+ ]))?;📝 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.
| let ds = vec![ | |
| 1704067200, 1704871384, 1705675569, 1706479753, 1707283938, 1708088123, 1708892307, | |
| 1709696492, 1710500676, 1711304861, 1712109046, 1712913230, 1713717415, | |
| ]; | |
| let y = vec![ | |
| 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0, 11.0, 12.0, 13.0, | |
| ]; | |
| let data = TrainingData::new(ds, y.clone())? | |
| .with_regressors(HashMap::from([ | |
| ( | |
| "foo".to_string(), | |
| vec![ | |
| 1.0, 2.0, 1.0, 2.0, 1.0, 2.0, 1.0, 2.0, 1.0, 2.0, 1.0, 2.0, 1.0, | |
| ], | |
| ), | |
| ( | |
| "bar".to_string(), | |
| vec![ | |
| 4.0, 5.0, 6.0, 4.0, 5.0, 6.0, 4.0, 5.0, 6.0, 4.0, 5.0, 6.0, 4.0, | |
| ], | |
| ), | |
| ])) | |
| .unwrap(); | |
| let ds = vec![ | |
| 1704067200, 1704871384, 1705675569, 1706479753, 1707283938, 1708088123, 1708892307, | |
| 1709696492, 1710500676, 1711304861, 1712109046, 1712913230, 1713717415, | |
| ]; // Unix timestamps starting from 2024-01-01 | |
| let y = vec![ | |
| 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0, 11.0, 12.0, 13.0, | |
| ]; // Example values with linear growth | |
| let data = TrainingData::new(ds, y.clone())? | |
| .with_regressors(HashMap::from([ | |
| ( | |
| "foo".to_string(), | |
| vec![ | |
| 1.0, 2.0, 1.0, 2.0, 1.0, 2.0, 1.0, 2.0, 1.0, 2.0, 1.0, 2.0, 1.0, | |
| ], | |
| ), | |
| ( | |
| "bar".to_string(), | |
| vec![ | |
| 4.0, 5.0, 6.0, 4.0, 5.0, 6.0, 4.0, 5.0, 6.0, 4.0, 5.0, 6.0, 4.0, | |
| ], | |
| ), | |
| ]))?; |
sd2k
force-pushed
the
wasmstan-example
branch
from
ae418f6
to
fa6bc96
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
crates/augurs-prophet/build.rs (2)
107-122: Consider adding a warning about the feature flag in the crate documentation.The
internal-ignore-build-failuresfeature is well documented in the code comments, but since it's a "hack" (as noted in the comments) that affects runtime behavior, consider adding a warning in the crate's documentation to ensure users don't enable it accidentally.
158-181: Consider reducing duplication in error handling logic.The error handling pattern is duplicated between
handle_cmdstanandhandle_wasmstan. Consider extracting the common logic into a shared function:fn handle_build_errors<T>( result: Result<(), T>, empty_files: &[&str], ) -> Result<(), Box<dyn std::error::Error>> { #[cfg(feature = "internal-ignore-build-failures")] if result.is_err() { create_empty_files(empty_files)?; } #[cfg(not(feature = "internal-ignore-build-failures"))] { if std::env::var("DOCS_RS").is_ok() { create_empty_files(empty_files)?; } else { result.map_err(|e| Box::new(e) as Box<dyn std::error::Error>)?; } } Ok(()) }This would simplify both handler functions and make the code more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
.github/workflows/rust.yml(4 hunks)crates/augurs-prophet/Cargo.toml(2 hunks)crates/augurs-prophet/build.rs(4 hunks)examples/forecasting/Cargo.toml(1 hunks)examples/forecasting/examples/prophet_wasmstan.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/rust.yml
- examples/forecasting/Cargo.toml
- examples/forecasting/examples/prophet_wasmstan.rs
🔇 Additional comments (3)
crates/augurs-prophet/Cargo.toml (1)
69-73: LGTM! Clear and accurate feature documentation.
The rename from internal-ignore-cmdstan-failure to internal-ignore-build-failures better reflects its broader scope, now covering both cmdstan compilation and wasmstan copying failures. The documentation clearly warns about proper usage.
crates/augurs-prophet/build.rs (2)
52-60: LGTM: Improved error handling for file operations.
The enhanced error messages now include both source and destination paths, making it easier to diagnose file copying issues during the build process.
Also applies to: 66-74
128-145: LGTM: Proper build caching and error handling for WASM.
Good addition of the rerun-if-changed directive and consistent error handling pattern.
| include = [ | ||
| ".gitignore", | ||
| "Cargo.toml", | ||
| "README.md", | ||
| "LICENSE-APACHE", | ||
| "LICENSE-MIT", | ||
| "CHANGELOG.md", | ||
| "build.rs", | ||
| "data", | ||
| "src/**/*", | ||
| "examples", | ||
| "tests", | ||
| "benches", | ||
| "prophet-wasmstan.wit", | ||
| "prophet-wasmstan.wasm", | ||
| "prophet.stan", | ||
| ] |
There was a problem hiding this comment.
💡 Codebase verification
Incorrect file paths in include section
The include section in Cargo.toml lists files that don't match their actual locations:
prophet-wasmstan.witis actually located atcomponents/cpp/prophet-wasmstan/wit/prophet-wasmstan.witprophet-wasmstan.wasmis not found in the repository- Only
prophet.stanis correctly located relative to the Cargo.toml
The include paths need to be updated to reflect the correct relative paths or the files need to be moved to match the current include configuration.
🔗 Analysis chain
Verify the presence of required WASM files.
The include section appropriately lists the necessary files for the Prophet WASM implementation. However, let's verify that the critical WASM files exist in the repository.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of required WASM files
# Expected: Both files should exist in the repository
echo "Checking for WASM files..."
for file in "prophet-wasmstan.wit" "prophet-wasmstan.wasm"; do
if fd -t f "^${file}$" .; then
echo "✓ Found: ${file}"
else
echo "✗ Missing: ${file}"
fi
done
Length of output: 458
Script:
#!/bin/bash
# Let's verify if the paths in include section match the actual file locations
# First, check the actual location of the WASM files relative to the Cargo.toml
echo "Checking relative paths from augurs-prophet Cargo.toml..."
cd crates/augurs-prophet 2>/dev/null || echo "Not in expected directory structure"
for file in "prophet-wasmstan.wit" "prophet-wasmstan.wasm" "prophet.stan"; do
echo -n "Looking for ${file} relative to Cargo.toml... "
if [ -f "${file}" ]; then
echo "Found in current directory"
else
echo "Not found in current directory"
fi
done
echo -e "\nActual locations in repository:"
cd - >/dev/null
fd -t f "prophet-wasmstan.wit|prophet-wasmstan.wasm|prophet.stan$"
Length of output: 1297
Adds an example of using Prophet with the wasmstan optimizer.
Summary by CodeRabbit
New Features
augursdependency to include theprophet-wasmstanfeature.Bug Fixes
Documentation
Chores
includesection in theCargo.tomlfor comprehensive file inclusion.