Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minimal MeTTa interpreter written mostly in Rust #728

Merged
merged 31 commits into from
Jul 10, 2024

Conversation

vsbogd
Copy link
Collaborator

@vsbogd vsbogd commented Jul 5, 2024

"old_interpreter" feature is added to switch back to the Rust interpreter if needed.
New metta operation is added into minimal interpreter in order to call Rust MeTTa interpreter implementation. Rust interpreter is fixed to consume metta operation passed. Interpreter unit tests are fixed to be compatible with minimal MeTTa interpreter.
Legacy common/arithmetics.rs modules is removed.
Old MeTTa interpreter written in MeTTa is removed.

vsbogd added 22 commits July 2, 2024 12:40
It is needed because grounded atoms doesn't take into account bindings
contents.
old_interpreter feature can be used to switch back to the previous Rust
interpreter. Add (interpret atom type space) to the old interpreter for
compatibility. Remove previous minimal MeTTa interpreter. Fix unit tests
which depends on a bare interpreter.
Add Python atom to designate MeTTa interpreter call and use it in unit
tests.
@vsbogd vsbogd requested a review from Necr0x0Der July 5, 2024 10:29
@vsbogd vsbogd requested a review from luketpeterson July 9, 2024 09:15
@luketpeterson
Copy link
Contributor

luketpeterson commented Jul 10, 2024

This speedup is huge! 50x in some of the interpreter benchmarks. I'll work on the line-by-line review, but I am very happy with the performance boost.

Update

Not only is the new interpreter much faster, but the scaling properties are better for recursion. The old interpreter(s) paid quadratic cost, this one is almost linear.

luketpeterson
luketpeterson previously approved these changes Jul 10, 2024
@@ -1,9 +1,12 @@
//! Contains MeTTa specific types, constants and functions.

pub mod text;
#[cfg(feature = "old_interpreter")]
pub mod interpreter;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the interpreter files be renamed? Now that interpreter_minimal is default...

Honestly I think it doesn't matter because there is probably not much reason to keep the old interpreter much longer. Speed was the biggest reason, but the new interpreter is much faster than the old one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Luke, it is a nice point. I renamed interpreter.rs to old_interpreter.rs (see 2bd7502) but there is also part of stdlib.rs related to the old interpreter. I think it will be simpler to refactor this in a one step when old interpreter is being removed. Anyway old_interpreter.rs sounds more clear and aligned with old_interpreter feature.

I am still not sure if we need to rename interpreter_minimal.rs to the interpreter.rs. Minimal MeTTa is has evaluation order which is different from MeTTa evaluation order. I think in the future we could allow more flexible evaluation control in MeTTa and then minimal MeTTa is not needed. On the other hand main structure of the interpreter (except the set of instructions) probably will be the same. By this reason I would prefer keep it as interpreter_minimal.rs for a while.

Copy link
Contributor

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the docs in docs/minimal-metta.md still accurate?

Yes, previous implementation was not changed. But you are right it is worth mentioning Rust implementation of the MeTTa interpreter. I added a note about it in 8cae12e. Also some variable scope questions are more clear now and I think I will add documentation for it in a separate PR.

I tried to render a diagram from the code using GPT.

Interesting experiment. Not sure what is simpler though: to draw new diagram or to finish this one :-)

@vsbogd vsbogd requested a review from luketpeterson July 10, 2024 08:12
@vsbogd vsbogd merged commit 4e56a8c into trueagi-io:main Jul 10, 2024
2 checks passed
@vsbogd vsbogd deleted the rust-minimal branch July 10, 2024 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants