Ramalama Run Doesnt have an Output

[bmahabirbu]

A came across an interesting issue where I wasn't getting an output anymore for ramalamam run. I found that in Commit f180b13 model.py line 230 was changed from using exec_cmd to run_cmd. What happens is run_cmd uses subprocess.run which doesn't pipe the output to the terminal (as its piped to stdout) while os.execvp does.

The line can be switched back to return the output back to terminal.

Im wondering if this was changed to fix a bug. If so maybe we can unify exec_cmd and run_cmd? Subprocess.run's output can be on the terminal. I know that there are a few differences to pick one over the other as subprocess creates a child instance while os.execvp overrides the python script.

[ericcurtin]

The reason we used exec in the past, is it simplifies things, so we replace the whole process with a "podman run --rm" or the uncontainerised version. There's no need to fork another process and it's simpler not to fork.

[bmahabirbu]

Makes sense! Then passing stdout=None will fix it if we don't want to use exec again.

run_cmd(conman_args, debug=args.debug, stdout=None)

[ericcurtin]

I was suggesting we do use exec again, there's no need to fork here and manage two processes, but if we want to do via run_cmd I guess it's fine.

[bmahabirbu]

Ah, I misunderstood. I agree we should use exec again instead of run_cmd for running exec_model_in_container. I'm with you, at that stage there is no reason to fork another process once we execute the main command.

[rhatdan]

If you can change it to exec and it does not break any tests then fine. The issue I was trying to catch is when the llama.cpp or vLLM does not exists in the container, to give a decent error to the user.

Try running a ramalama with a --image fedora and see the error you get if the llama.cpp file is not in the image, if the error looks ok to a naive user, then we can stay with exec. My goal was to tell them that the image had to have llama.cpp installed in it for ramalama to work with it.

[rhatdan]

I would take usability over worrying about another fork/exec anyday.

[ericcurtin]

Ah... The lack of errors is probably because we have to redirect stderr to null because of all the llama-cli verboseness. When we move to the recently merged llama-run alternative, we can remove that stderr to null hack. llama-run is also prettier, colorised and has less bugs.

[rhatdan]

Please open a PR to do this ASAP and then we can get a new release out.

[vpavlin]

I started playing with Ramalama yesterday and hit this issue as well:) Glad it is already reported and being looked into!