Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow configuring a default model for cell magics (and line error magic) #962

Merged
merged 2 commits into from
Aug 23, 2024

Conversation

krassowski
Copy link
Member

The name and help for default_language_model is copied over from the one used in equivalent jupyter-ai traitlet.

@krassowski krassowski added bug Something isn't working enhancement New feature or request and removed bug Something isn't working labels Aug 19, 2024
@haesleinhuepf
Copy link

Awesome @krassowski! Fantastic to see this happening.

Just out of curiosity: Would it be possible to make the syntax more intuitive, e.g. instead of this:

%config AiMagics.default_language_model = "anthropic:claude-v1.2"

One could also say

%ai set default llm to anthropic:claude-v1.2

That's human readable and easy to memorize, even though could be even shorter...

@krassowski
Copy link
Member Author

I would argue that %config is preferred as this is the standard way to change configuration in IPython, see:

%ai magic uses click parser and syntax that you propose would not be easy to implement. I think it might be worth considering as a separate enhancement.

Copy link
Collaborator

@srdas srdas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krassowski - Looks great! Have wanted this for some time - TY. I tried various things to break it - but it's all good.
image
Note: Why not default to the Text Completion LLM set in Chat always (if there is one set), and only use the %config magic if you want to override and use a different default LLM for the magics?

The documentation add-ons are also good.

Code to correct cell magic use of shell looks good.

@krassowski
Copy link
Member Author

Note: Why not default to the Text Completion LLM set in Chat always (if there is one set), and only use the %config magic if you want to override and use a different default LLM for the magics?

Maybe in a future PR. It is rather complex quest because the chat and ai magics do not necessarily live in the same environment (may be physically different computers). While chat imports a copy of the magics package, it is potentially a different copy than the one used in the kernel. There are ways to solve this, I explored some of the ideas in the linked issue.

@srdas
Copy link
Collaborator

srdas commented Aug 22, 2024

A few more checks on the PR:

  1. Check that setting the LLM with magics %config does not overwrite the specified chat LLM.✅
  2. Does setting the default LLM in magics also carry over to a new notebook in the same Jupyter session? No it does not, which is good so that each notebook can have it's own default LLM for magics. ✅
  3. If you put in a non-existent LLM as default, check that it fails. Then if we use the %%ai magics with a different LLM and then go back to default, make sure that the default is not overwritten. ✅
    image
    All looks good.

@@ -257,7 +265,7 @@ def _is_langchain_chain(self, name):
if not acceptable_name.match(name):
return False

ipython = get_ipython()
ipython = self.shell
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krassowski
I am not deeply familiar with magics, curious if this change has any implications or is a drop-in replacement for the existing statement?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is drop-in replacement, except that it now can be properly tested and used outside of IPython interpreter. There is a reason why Magics constructor receives the shell argument. get_ipython() should never be used in magics.

@3coins 3coins merged commit e45eff5 into jupyterlab:main Aug 23, 2024
8 checks passed
@krassowski krassowski deleted the default-magics-model branch August 23, 2024 06:30
Marchlak pushed a commit to Marchlak/jupyter-ai that referenced this pull request Oct 28, 2024
…ic) (jupyterlab#962)

* Allow configuring a default model for cell magics (and line error magic)

* Add documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cell magic uses incorrect IPython shell Default model for %%ai magic command
4 participants