trueagi-io / hyperon-experimental

MeTTa programming language implementation
https://metta-lang.dev
MIT License
133 stars 44 forks source link

Update REPL sections of README.md #598

Closed vsbogd closed 6 months ago

astroseger commented 6 months ago

@vsbogd since you've asked me to review this. I've tested README instruction in Dockerfile (modified docker from repo). And I got error on much earlier step (cargo test).

FROM ubuntu:22.04

RUN apt-get update && \
    DEBIAN_FRONTEND=noninteractive \
    TZ=UTC \
    apt-get install -y sudo git python3 python3-pip curl gcc cmake && \
    rm -rf /var/lib/apt/lists/*

RUN echo '%sudo ALL=(ALL) NOPASSWD:ALL' >> /etc/sudoers
RUN useradd -m -g users user
RUN usermod -aG sudo user
USER user
ENV HOME=/home/user
WORKDIR ${HOME}

RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs > /tmp/rustup.sh
RUN sh /tmp/rustup.sh -y && rm /tmp/rustup.sh
ENV PATH="${PATH}:/home/user/.cargo/bin"
ENV PATH="${PATH}:/home/user/.local/bin"

ADD . /home/user/hyperon
RUN sudo chown -R user.users /home/user/hyperon 
WORKDIR ${HOME}/hyperon

# FROM README
RUN cargo install cbindgen
RUN python3 -m pip install conan==1.62 pip==23.1.2
RUN conan profile new --detect default
RUN python3 -m pip install pip==23.1.2
RUN cargo test

It return error:

......
   Compiling heck v0.4.1
error[E0658]: use of unstable library feature 'ptr_addr_eq'
   --> lib/src/space/mod.rs:354:9
    |
354 |         std::ptr::addr_eq(RefCell::as_ptr(&self.0), RefCell::as_ptr(&other.0))
    |         ^^^^^^^^^^^^^^^^^
    |
    = note: see issue #116324 <https://github.com/rust-lang/rust/issues/116324> for more information

   Compiling clap_builder v4.5.1
   Compiling radix_trie v0.2.1
   Compiling parking_lot v0.12.1
   Compiling fd-lock v4.0.2
For more information about this error, try `rustc --explain E0658`.
error: could not compile `hyperon` (lib) due to previous error
warning: build failed, waiting for other jobs to finish...
luketpeterson commented 6 months ago

This change to use ptr::addr_eq was made in response to Rust 1.76 being released which stabilized the ptr::addr_eq API and added a warning to checking pointer-equality without also checking pointer metadata.

Is it possible you are pulling an older version of the Rust toolchain?

astroseger commented 6 months ago

This change to use ptr::addr_eq was made in response to Rust 1.76 being released which stabilized the ptr::addr_eq API and added a warning to checking pointer-equality without also checking pointer metadata.

Is it possible you are pulling an older version of the Rust toolchain?

It should be reflected in README. I've emulated user who simply follow instruction in README (by simply running them without reading the text, because user wants only metta + repl and don't want to go into detail of process of rust installation)

luketpeterson commented 6 months ago

Personally I think this covers that: https://github.com/trueagi-io/hyperon-experimental/blob/02fc0c2100bb81e957c262c73a27d54f6d406465/README.md?plain=1#L210

My guess is you are hitting the error because you have an existing, but old, version of the rust tools. A brand new user will fetch the latest tools when they initialize their setup following the earlier instructions. And a user with an existing rust tool chain will hopefully read the part of the readme that tells the user to update their rust tools if they hit errors.

At least that's the way I hope it would go.

vsbogd commented 6 months ago

@astroseger , probably the issue with docker you describe arises because you have old cached docker image. Could you please try building it with --no-cache. And if it helps please explain how do you suggest to update README.md to take it into account?

astroseger commented 6 months ago

Sorry that it took me so long to replay. Yes, it was problem with cache. However now I have the following error:

Step 22/30 : RUN cargo run --bin metta
 ---> Running in 36b728050f7e
   Compiling metta-repl v0.1.7 (/home/user/hyperon/repl)
    Finished dev [unoptimized + debuginfo] target(s) in 1.35s
     Running `target/debug/metta`
Fatal Error: ModuleNotFoundError: No module named 'hyperon'

The docker file for reference

FROM ubuntu:22.04

RUN apt-get update && \
    DEBIAN_FRONTEND=noninteractive \
    TZ=UTC \
    apt-get install -y sudo git python3 python3-pip curl gcc cmake && \
    rm -rf /var/lib/apt/lists/*

RUN echo '%sudo ALL=(ALL) NOPASSWD:ALL' >> /etc/sudoers
RUN useradd -m -g users user
RUN usermod -aG sudo user
USER user
ENV HOME=/home/user
WORKDIR ${HOME}

RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs > /tmp/rustup.sh
RUN sh /tmp/rustup.sh -y && rm /tmp/rustup.sh
ENV PATH="${PATH}:/home/user/.cargo/bin"
ENV PATH="${PATH}:/home/user/.local/bin"

ADD . /home/user/hyperon
RUN sudo chown -R user.users /home/user/hyperon 
WORKDIR ${HOME}/hyperon

# FROM README
RUN cargo install cbindgen
RUN python3 -m pip install conan==1.62 pip==23.1.2
RUN conan profile new --detect default
RUN python3 -m pip install pip==23.1.2
RUN cargo test
RUN cargo run --example sorted_list
RUN cargo run --bin metta
RUN RUST_LOG=hyperon=debug cargo test
RUN cargo +nightly bench
RUN cargo doc --no-deps
RUN mkdir -p build
RUN WORKDIR ${HOME}/hyperon/build
RUN cmake ..
RUN make
RUN make check
astroseger commented 6 months ago

Sorry, actually the error is as follows:

Step 24/30 : RUN cargo +nightly bench
 ---> Running in bb5e8155a82f
error: toolchain 'nightly-x86_64-unknown-linux-gnu' is not installed

But I'm not sure that we need to check README in this manner, I simply don't know the better way. The error with

Fatal Error: ModuleNotFoundError: No module named 'hyperon'

has been fixed, so in principle PR has fixed this particular problem.

vsbogd commented 6 months ago

has been fixed, so in principle PR has fixed this particular problem.

Thanks you for the confirmation.

error: toolchain 'nightly-x86_64-unknown-linux-gnu' is not installed But I'm not sure that we need to check README in this manner

Yes, to me it is a question should the README.md be the document which contains a complete list of commands for each case. Usually I don't do this. Trying to completely explain how to run Rust benchmarks ends up with including a lot of Rust documentation into the README.md. This documentation duplicates Rust's one and finally we get to the point when we need updating README.md after Rust documentation is updated. On the other hand I include some specific command more as a note for a quick search. For example one can grep bench README.md to quickly find a command once environment is ready.

Summarizing this I more like removing benchmark command from the README.md rather then adding new command which explains how to install nightly toolchain. On the other hand user should understand what he is doing. Thus if one runs benchmark then probably he wants to improve performance. If he wants to improve performance then he probably knows how to use Rust tools. If he knows how to use Rust tools then he knows how to install nightly toolchain. Otherwise it is not clear why one wants to run the benchmark.