kinode-dao / kinode

Kinode OS runtime
https://kinode.org
Apache License 2.0
35 stars 13 forks source link

terminal: improve `m` DexX? #260

Open nick1udwig opened 8 months ago

nick1udwig commented 8 months ago

Currently, using m requires an intimate knowledge of the API of whatever process you are interacting with -- and it fails mysteriously if you make a mistake.

This is illustrated by the no-ui file_transfer package. To call it successfully:

fake.os > m fake2.os@foo:foo:template.os "ListFiles" -a 5
Mon 2/19 09:14 m: awaiting response for 5s
Mon 2/19 09:14 {"ListFiles":[{"name":"foo:template.os/files/Great Is Thy Faithfulness.m4a","size":3145134}]}
fake.os > m our@foo:foo:template.os '{"Download": {"name": "Great Is Thy Faithfulness.m4a", "target": "fake2.os@foo:foo:template.os"}}' 
Mon 2/19 09:22 foo worker: begin
Mon 2/19 09:22 file: Great Is Thy Faithfulness.m4a progress: 33%
Mon 2/19 09:22 file: Great Is Thy Faithfulness.m4a progress: 66%
Mon 2/19 09:22 foo worker done: exiting, took 89.831645ms
Mon 2/19 09:22 file: Great Is Thy Faithfulness.m4a progress: 100%

So you need to know that you have to await one and not the other. The fail cases look like:

fake.os > m fake2.os@foo:foo:template.os "ListFiles"
Mon 2/19 09:37 event loop: don't have 7640537613494437609:terminal:sys amongst registered processes (got message for it from network)
fake.os > m our@foo:foo:template.os '{"Download": {"name": "Great Is Thy Faithfulness.m4a", "target": "fake2.os@foo:foo:template.os"}}' -a 5
Mon 2/19 09:37 m: awaiting response for 5s
Mon 2/19 09:37 foo worker: begin
Mon 2/19 09:37 file: Great Is Thy Faithfulness.m4a progress: 33%
Mon 2/19 09:37 file: Great Is Thy Faithfulness.m4a progress: 66%
Mon 2/19 09:37 foo worker done: exiting, took 93.304272ms
Mon 2/19 09:37 file: Great Is Thy Faithfulness.m4a progress: 100%
Mon 2/19 09:37 m: SendError: Timeout

The second case is more graceful: it does what I want it to, but then prints some error message.

We should either

  1. Improve m DevX so that "it just works" (preferred, if possible)
  2. Add error messaging that is very clear, e.g., in the ListFiles case, perhaps something like Responses must be awaited; try -a 5.

I understand there are challenges to both approaches, but we should think how to make the DevX better.

One possibility is to make -a 5 the default behavior and then hide that m: SendError: Timeout for the specific case of "-a 5 default". This would mean that cases like the ListFiles would "just work" -- as would non-awaited cases. The downside here is that this does not extend generally to longer await times.

FYI @tadad

dr-frmr commented 8 months ago

I am wary of adding default beheavior here, maybe just clearer error messages. I think people should have to generally understand the protocol they are sending raw messages to. Another factor here is that if people are actually writing TUIs, they should ensconce their actions in scripts. So you won't be using m to hit TUIs often at all unless you're testing/debugging an app.

tadad commented 8 months ago

I had a good solution to making m "just work" - but this no longer works because of a recent change that drops responses if the req didn't expects_response. We could change this behavior so that instead of a silent drop, it gives an error to the process like got unexpected resopnse from foo:bar:baz.

If that's not a good option for some reason, then I think I could update the kernel error messages to be more informative

tadad commented 8 months ago

after a short reflection I don't think it makes sense to give unexpected response errors to processes - the kernel should drop those automatically. Going to improve the kernel error messages now

tadad commented 8 months ago

261 would have the above errors read as:

Old:

fake.os > m fake2.os@foo:foo:template.os "ListFiles"
Mon 2/19 09:37 event loop: don't have 7640537613494437609:terminal:sys amongst registered processes (got message for it from network)

New:

fake.os > m fake2.os@foo:foo:template.os "ListFiles"
Mon 2/19 09:37 event loop: got Response from network for 7640537613494437609:terminal:sys, but process does not exist (perhaps it terminated)

Old:

m: SendError: Timeout

New:

m: failed to send message due to timeout
nick1udwig commented 8 months ago

So the problem with these errors is that they don't tell me, a user, what I should do here.

The Rust compiler errors are a good model of something to shoot for. They tell you "this went wrong", but also often have a "hint" that tells you what you can try in order to address the error. So we should aim for an error here that is tailored to this specific case if it is at all possible to do so, and it should suggest to the user something like "did you forget to await the response? Try m ... -a 5" or something like that.

dr-frmr commented 8 months ago

Yep, just adding that "hint" to the terminal error messages seems like enough here

tadad commented 8 months ago

fixed & merged!

nick1udwig commented 8 months ago

Can we do better here wrt targeting the hint? I don't like printing to a user "if this then this". Can we do this with code? E.g. check target/source and only print if it is targeted to m or something?

tadad commented 8 months ago

unfortunately not

nick1udwig commented 8 months ago

After looking more closely, I agree my proposed solution won't work since we don't have the process_name of m. However, we certainly can do better than just always hinting. E.g., we could only give this hint if the package is terminal:sys. Can we do even better than that?

nick1udwig commented 7 months ago

Bump

tadad commented 7 months ago

yes I think we could do that but I'm unsure if we want to have the kernel "know" about terminal:sys and hardcode that process id into error message printing. If we allow ourselves to do that, then yes I can make the errors much better

nick1udwig commented 7 months ago

We've already hardcoded it in the error string. This would be adding a conditional, which is in some sense different and in some sense the same.