Use PeTTa as interpreter#3
Conversation
|
My bad - I closed this PR by accident. |
|
@rmgaray I am still in the process of coming up to speed on everything. However, I have a moderate understanding now and was able to give this a review, relying heavily on Claude and Codex. The following is everything flagged. I haven't been able to verify everything fully yet but it does seem like we need to at least address the non determinism, security concerns (even if just gating behind some clearly dangerous feature flag), and documentation/test gaps. PR #3 Review — "Use PeTTa as interpreter" (commit
|
|
|
I've added 3 new issues referenced above for tracking things flagged in this PR review that we will be addressing later (whether planned or requiring further consideration). |
|
|
||
| ## What the Review Asked For | ||
|
|
||
| From the original review: |
There was a problem hiding this comment.
Please remove this "What the Review Asked For" section. It is in response to our internal review which won't be relevant for future readers
There was a problem hiding this comment.
I'm embarrassed to say that I accidentally checked in this file. I intended only the doc comments in the Rust code to serve as documentation.
Since you found this file useful, I will keep it - but I will improve the AI writing which is awful.
| From the original review: | ||
| > No unit/integration tests added. The milestone explicitly calls for "tests and example Rholang scripts to demonstrate that the … execution is occurring." Only an example is present; no Rust-side test exercises petta_execute, no cargo test coverage of value_to_par, and crucially no replay test — which is what would have caught the missing non_deterministic_ops() entry. | ||
|
|
||
| **Our Implementation:** |
There was a problem hiding this comment.
This "Our Implementation" section seems to repeat information covered above. This whole doc is likely to become stale quickly so consider turning it into a shorter doc that provides readers with broader details like how to run the tests, etc. as opposed to the current state (which can go in the PR description).
|
|
||
| #[tokio::test] | ||
| async fn test_petta_execute_simple_swap() { | ||
| if !petta_available() { |
There was a problem hiding this comment.
This is minor, but there's many !petta_available() checks with the same message. And this provides no way to require these tests to run (e.g., in a strict CI environment).
Consider using a helper function something like this:
fn should_skip_petta_test() -> bool {
let require = env::var_os("REQUIRE_PETTA_TESTS").is_some();
let petta_path = PathBuf::from(env::var("PETTA_PATH").unwrap_or("./PeTTa".into()));
let metta_module_path = petta_path.join("src/metta.pl");
let petta_missing = !metta_module_path.exists();
let swipl_missing = Command::new("swipl")
.arg("--version")
.output()
.map(|output| !output.status.success())
.unwrap_or(true);
if !petta_missing && !swipl_missing {
return false;
}
let message = format!(
"PeTTa test prerequisites unavailable: metta.pl missing={}, swipl missing={}. Set PETTA_PATH and install swipl.",
petta_missing,
swipl_missing
);
if require {
panic!("{message}");
}
eprintln!("Skipping test: {message}");
true
}I deleted unnecessary details and improved the writing.
…when handle is dropped
|
@nrutledge I believe all comments should be addressed now. |
|
|
||
| let error_message: String; | ||
| match (petta_missing, swipl_missing) { | ||
| (false, false) => return true, |
There was a problem hiding this comment.
This seems like it should actually be (false, false) => return false (neither are missing, don't skip). Or am I misunderstanding.
| use std::path::PathBuf; | ||
| // Helper for skipping PeTTa tests if runtime pre-requisites are not met (or | ||
| // panicking if tests are mandatory). | ||
| fn should_skip_petta_test() -> bool { |
There was a problem hiding this comment.
Just checking, is the second should_skip_petta_test here implementation intentional?
This PR adds execution of MeTTa programs using swipl and PeTTa. The output of the execution of MeTTa programs is captured by f1r3fly, parsed as JSON and then passed to the Rho program as a native data structure.
Example: