panic when ollama doesn't have the model

[thomasaarholt]

I happened to have ollama running while setting up smartcat. It does not have the phi3 model installed. Here is the output from running sc for the first time:

❯ sc
Prompt config file not found at /Users/thomas/.config/smartcat/prompts.toml, generating one.
...
API config file not found at /Users/thomas/.config/smartcat/.api_configs.toml, generating one.
...
No API key is configured.
How to configure your API keys:
https://github.com/efugier/smartcat/#configuration

thread 'main' panicked at /Users/thomas/.cargo/registry/src/index.crates.io-6f17d22bba15001f/smartcat-2.1.0/src/utils.rs:12:9:
API request failed with status 404 Not Found: {"error":"model \"phi3\" not found, try pulling it first"}
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
zsh: abort      sc

I assume phi3 comes from the .api_configs content:

[ollama]
url = "http://localhost:11434/api/chat"
default_model = "phi3"
timeout_seconds = 180

[thomasaarholt]

I see that you often return panics when smartcat encounters an error. It would be nice to just return the error text instead of the full panic.

❯ sc
thread 'main' panicked at /Users/thomas/.cargo/registry/src/index.crates.io-6f17d22bba15001f/smartcat-2.1.0/src/text/api_call.rs:75:37:
version required for Anthropic, please add version key to your api config
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
zsh: abort      sc

[efugier]

Hey, thanks for the feedback!

It would be nice to just return the error text instead of the full panic.

I'm not sure what you mean by that and what would the end goal be. Are the error messages not clear enough?

[thomasaarholt]

Thank for your making a really cool piece of software!

Hehe, I just mean that instead of

❯ sc
thread 'main' panicked at /Users/thomas/.cargo/registry/src/index.crates.io-6f17d22bba15001f/smartcat-2.1.0/src/text/api_call.rs:75:37:
version required for Anthropic, please add version key to your api config
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
zsh: abort      sc

I would expect (and perhaps in red)

❯ sc
Error:
version required for Anthropic, please add version key to your api config

If this is a lot of work, then don't worry about it. This is "nice to have". Most people will figure out how to fix from the original error - it's just that when I got a panic, my first thought was "uh oh, something is wrong with smartcat, not with my config". Perhaps especially when I got the panic in my main message above, which was the very first time I ran smartcat.

[efugier]

it's just that when I got a panic, my first thought was "uh oh, something is wrong with smartcat

Oh interesting! Is that an acknowledged pattern or were you unfamiliar with rust panics?

I am not super versed in the best practices of error handling in rust, but I was under the impression that when the error is final and would induce an unrecoverable state in your program, calling a panic to exit with an error message was the thing to do: https://doc.rust-lang.org/book/ch09-03-to-panic-or-not-to-panic.html

I checked the cases where I used panic! and they all fit that definition in my eyes: what I want is an "exit now with this message and tell the shell it is an error" function, and I don't know of another that would fit the bill and produce a more appealing output.
I know about using Result and passing the errors along, but it seemed like overengineering in those cases.

I agree that panics may read weird at first but I've always seen them as simply errors in rust with a bit a extra text around them give some insight. I genuinely don't know if that's a fallacy 🤔

Happy to read your thoughts on that matter!

[thomasaarholt]

I am definitely not an expert on the matter! I am mostly familiar with rust through polars, where the main author treats panics as bugs. I can't find a reference there right now though. But even though polars does it, it doesn't mean that this is the way™.

Your link was a good one. I agree that it looks like you have it the right way. I wish that that page included more examples including "panics that tell the user how to fix a situation", since I do find it a bit odd that this is totally accepted behaviour.

All that said, I think I would leave things mostly as they are in smartcat, but perhaps try to ensure that when panics do happen, the error should be clearly distinguishable from the rust debug messages. Not exactly sure how to do that, though 🤔

[efugier]

Not exactly sure how to do that, though 🤔

Neither am I! I know of a few crates that prettify the panic output but I usually try to keep dependencies that aren't absolutely required to a minimum but I might consider it.

I'll look inside popular and established rust tool to see how they do 🙂

[thomasaarholt]

Sounds good! Thanks for a nice discussion!