Make path argument required on /learn

[andrewfulton9]

closes #819

This PR adds a dialog pop-up when /learn is used without a directory parameter. This makes it clear what is happening and gives people the opportunity to cancel the operation if learning on all files under the root (or preferred) directory wasn't intended.

Updated to make /learn require a path argument. Now /learn will respond with a helpful message and example input if no path argument is given.

[krassowski]

Can you apply a label now?

[dlqqq]

@andrewfulton9 I'm not sure if this feature is the best user experience we can offer here. If a user runs /learn without specifying a target directory, why should that trigger a dialog that takes over the user's entire screen? This is a minor usage error that can be displayed as a reply in the chat panel.

[andrewfulton9]

@andrewfulton9 I'm not sure if this feature is the best user experience we can offer here. If a user runs /learn without specifying a target directory, why should that trigger a dialog that takes over the user's entire screen? This is a minor usage error that can be displayed as a reply in the chat panel.

@dlqqq, thanks for the feedback! My initial attempt at this was actually doing that. My concern with having it be a reply in the chat panel, at least if its asking for confirmation and not just saying an argument is required, is that the chat interactions aren't necessarily synchronous so you could have a situation where you have a history that looks like:

[
   "/generate a notebook that gives basic usage examples of numpy",
   "Great, I will get started on your notebook. It may take a few minutes, but I will reply here when the notebook is ready. In the meantime, you can continue to ask me other questions.",
   "/learn ",
   "Can you confirm that you want to learn under all files in the directory. if so,type yes",
   "I have created your notebook and saved it to the location /home/vagrant/jupyter-ai/Basic Usage and Operations with NumPy in Python.ipynb. I am still learning how to create notebooks, so please review all code before running it.",
    "yes"
]

So in this situation, the code will have to check every incoming message to see if it is "yes" and probably some variations or that ("Yes") and then determine what that was a response to. Maybe in practice this wouldn't be as problematic as I expect, but I could see there being lots of confusing edge cases.

Maybe the best answer here is to just not allow empty directory arguments in the first place. What do you think? Should I take a stab at in chat panel confirmation, allowing no empty directory argument, continue with front-end implementation, or another option that I'm not thinking of?

[dlqqq]

@andrewfulton9 There's no reliable way I can think of that allows a user to reply yes/no to a previous question, since commands are only routed to the LearnChatHandler if the user is calling /learn. Even if we allow some kind of syntax like /learn yes (replying to a previous "question" coming from the LLM), it's ambiguous as to whether it should confirm or learn a directory literally named yes/.

It seems the root issue is that /learn replies with an opaque, user-unfriendly message when it is called without a target directory.

Can /learn be modified to produce a better error message when no argument is specified? The error message should state that /learn requires an argument specifying the target directory, and also should provide examples like /learn .. For reference, this block of code in learn.py is what triggers the reply when no arguments are passed:

        # Make sure the path exists.
        if not len(args.path) == 1:
            self.reply(f"{self.parser.format_usage()}", message)
            return

Replying with a better error message here is a simpler solution. It avoids opening a full-page dialog that interrupts the user's workflow, and does not require any response. The user can read the error message and determine what command they want to run by themselves.

[richlysakowski]

The user must have an option to explicitly specify the name of the notebook being generated. This is very important because we sometime see invalid filenames generated by AI. This results in no file being written out.

…

On Tue, Sep 24, 2024, 1:34 PM Andrew Fulton ***@***.***> wrote: @andrewfulton9 <https://github.com/andrewfulton9> I'm not sure if this feature is the best user experience we can offer here. If a user runs /learn without specifying a target directory, why should that trigger a dialog that takes over the user's entire screen? This is a minor usage error that can be displayed as a reply in the chat panel. @dlqqq <https://github.com/dlqqq>, thanks for the feedback! My initial attempt at this was actually doing that. My concern with having it be a reply in the chat panel, at least if its asking for confirmation and not just saying an argument is required, is that the chat interactions aren't necessarily synchronous so you could have a situation where you have a history that looks like: [ "/generate a notebook that gives basic usage examples of numpy", "Great, I will get started on your notebook. It may take a few minutes, but I will reply here when the notebook is ready. In the meantime, you can continue to ask me other questions.", "/learn ", "Can you confirm that you want to learn under all files in the directory. if so,type yes", "I have created your notebook and saved it to the location /home/vagrant/jupyter-ai/Basic Usage and Operations with NumPy in Python.ipynb. I am still learning how to create notebooks, so please review all code before running it.", "yes" ] So in this situation, the code will have to check every incoming message to see if it is "yes" and probably some variations or that ("Yes") and then determine what that was a response to. Maybe in practice this wouldn't be as problematic as I expect, but I could see there being lots of confusing edge cases. Maybe the best answer here is to just not allow empty directory arguments in the first place. What do you think? Should I take a stab at in chat panel confirmation, allowing no empty directory argument, continue with front-end implementation, or another option that I'm not thinking of? — Reply to this email directly, view it on GitHub <#1012 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACHJVL3LNN4NXS2K24SRRPDZYGPDBAVCNFSM6AAAAABOW7CVKWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNZRHEYDMMZUGQ> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[JasonWeill]

@richlysakowski This pull request concerns the /learn command. If you'd like to add an option to the /generate command, please open a pull request that fixes #990.

[krassowski]

Caught a few typos, no other comments

[dlqqq]

@andrewfulton9 Love the new error message! It's a concise summary of exactly how to use /learn. Just two tiny suggestions and this is good to go.

[dlqqq]

@andrewfulton9 Awesome, thank you!