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

Settings section improvement #918

Merged
merged 15 commits into from
Aug 5, 2024

Conversation

andrewfulton9
Copy link
Contributor

Closes #903

This changes the settings section so that the dropdown is replaced by text indicating that the section assets are not available (API Keys, language models, embedded models). It also replaces the inline completer dropdown with text indicating if the inline completer is not enabled.

image

@srdas srdas added the enhancement New feature or request label Jul 30, 2024
Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

Thank you for working on this! See suggestions inline.

@@ -285,6 +274,147 @@ export function ChatSettings(props: ChatSettingsProps): JSX.Element {
);
}

let language_model_section;
Copy link
Member

Choose a reason for hiding this comment

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

I think snakeCase would be more consistent with the codebase here (and for the other one). Not sure what others think about using a variable vs keeping it inline with ternary vs moving it to a new functional component.

packages/jupyter-ai/src/components/chat-settings.tsx Outdated Show resolved Hide resolved
packages/jupyter-ai/src/components/chat-settings.tsx Outdated Show resolved Hide resolved
packages/jupyter-ai/src/components/chat-settings.tsx Outdated Show resolved Hide resolved
Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

Tested it locally, and I am not able to select model right now because the current version checks the current provider rather than all of them (and there are times when there is no provider configured, like when launching the extension for the first time).

For other reviewers, you can test the "all disabled state" this with

jupyter lab --AiExtension.allowed_providers=X --AiExtension.allowed_providers=X

which disallows all providers (needs to be repeated twice due to #913).

@@ -285,6 +274,150 @@ export function ChatSettings(props: ChatSettingsProps): JSX.Element {
);
}

let languageModelSection;
if (lmProvider?.chat_models && lmProvider.chat_models.length > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be iterated across all server.lmProviders, not only the current one, in a similar why to how the menu options are populated, see:

{server.lmProviders.providers.map(lmp =>
  lmp.models
    .filter(lm => lmp.chat_models.includes(lm))
    .map(lm => (
      <MenuItem value={`${lmp.id}:${lm}`}>
        {lmp.name} :: {lm}
      </MenuItem>
    ))
)}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be fixed now.

Comment on lines 349 to 350
lmProvider?.completion_models &&
lmProvider?.completion_models.length > 0
Copy link
Member

Choose a reason for hiding this comment

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

Same as in the chat one

@srdas
Copy link
Collaborator

srdas commented Jul 30, 2024

@andrewfulton9 TY for working on this. Please also update the related documentation for this enhancement.

I have also tested the changes and they seem to be working correctly, with one clarification needed, maybe @krassowski would know best. When only the "completions History provider" is enabled, the box on the left to enter the Inline completion model is grayed out, and is only active when the "completions Jupyter AI provider" is enabled and that box is checked as can be seen next:
image
When the "completions Jupyter AI provider" is checked then we can choose an Inline completions provider as shown here:
image
If this behavior is intended, we can ignore this comment.

@krassowski
Copy link
Member

When only the "completions History provider" is enabled, the box on the left to enter the Inline completion model is grayed out, and is only active when the "completions Jupyter AI provider" is enabled and that box is checked as can be seen next:

Yes, this is exepcted - the history provider is from core jupyterlab. The sidebar should only react to changes in "Jupyter AI provider" status.

Copy link
Member

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

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

@andrewfulton9 Thank you for this PR! Mike and I have left some feedback below to address.

In addition, rather than defining languageModelSection and inlineCompletionSection as variables storing JSX elements, can you handle the conditional logic directly within the return statement using ternary statements? This is like what is being done in lines 444-466 for the embedding models section. Doing so would improve readability IMO, since it does not require a reader to jump above to the definition of languageModelSection to determine how the output is being rendered.

@andrewfulton9
Copy link
Contributor Author

@srdas, thanks for reviewing! Do you have a suggestion on what in the docs should be updated? I don't see anything in the docs related to the settings panel. Maybe something in the "blocklisting providers" section? Though this PR doesn't actually do any configuration, it just makes the settings panel more clear when a there isn't a option available to select.

@srdas
Copy link
Collaborator

srdas commented Aug 4, 2024

@srdas, thanks for reviewing! Do you have a suggestion on what in the docs should be updated? I don't see anything in the docs related to the settings panel. Maybe something in the "blocklisting providers" section? Though this PR doesn't actually do any configuration, it just makes the settings panel more clear when a there isn't a option available to select.

Thanks for asking for clarification @andrewfulton9. As of now, inline completion is not discussed in the documentation, see this link: https://jupyter-ai.readthedocs.io/en/latest/users/index.html#the-chat-interface. In this section in the User documentation, after discussing the choice of Language model and Embedding model, the screenshots shown should be replaced by (i) showing inline completion as an option, (ii) showing the settings page for Inline Completion, (iii) explain the latest version of the inline completion setup incorporating the improvement in this PR. Wait to do this (and I'd be happy to help) after finalizing the changes in this PR based on comments from @dlqqq and @krassowski at which point the latest version of the UI can be used to update the documentation.

Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @andrewfulton9 !

Copy link
Member

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

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

@andrewfulton9 Thank you!

@dlqqq dlqqq merged commit 4d69962 into jupyterlab:main Aug 5, 2024
8 checks passed
This was referenced Aug 5, 2024
Marchlak pushed a commit to Marchlak/jupyter-ai that referenced this pull request Oct 28, 2024
* show no api keys needed for selected model

* checks for existing key in rendering so No API Keys message doesn't appear when key already exists

* Handle not having language and embedding models

* lint

* inline_completer

* Update packages/jupyter-ai/src/components/chat-settings.tsx

Co-authored-by: Michał Krassowski <[email protected]>

* Update packages/jupyter-ai/src/components/chat-settings.tsx

Co-authored-by: Michał Krassowski <[email protected]>

* Update packages/jupyter-ai/src/components/chat-settings.tsx

Co-authored-by: Michał Krassowski <[email protected]>

* review fixes

* addressing reviews

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* lint

---------

Co-authored-by: Michał Krassowski <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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.

Setting sections are slightly confusing when empty
4 participants