Setting default model providers #421

aws-khatria · 2023-11-01T15:59:26Z

This pull request introduces the setting of default model provider details in the Jupyter AI extension. The changes primarily involve extending the ConfigManager to initialize default values, and extension.py to initialize the values from CLI. Fixes #218.

The changes include:

Setting the model_provider_id
Setting the embedding_model_provider_id
Setting api_keys for corresponding model provider
Setting fields in cases where model provider requires additional details to model provider.
Support to merge default values provided in the CLI even when the config.json file exists. This is also tested using unit-test case.

Testing:

The changes were tested manually by running the Jupyter AI extension and verifying that the default model provider is correctly.

In addition to manual testing, unit tests are also included in this commit. The tests cover the changes made to the ConfigManager class and ensure that the default model provider details are correctly set.

These changes will improve the usability of the Jupyter AI extension by configuring the defaults for the model provider.

Command with which extension was tested:

jlpm dev --AiExtension.model_provider_id="bedrock-chat:anthropic.claude-v1"  --AiExtension.fields='{"bedrock-chat:anthropic.claude-v1":{"credentials_profile_name": "default","region_name": "us-west-2"},"bedrock:amazon.titan-embed-text-v1":{"credentials_profile_name": "default","region_name": "us-west-2"}}' --AiExtension.api_keys='{"OPENAI_API_KEY": "sk-xyz"}'

Executed unit-test using jlpm test command.

Next steps:

Review the changes and provide feedback.
Add relevant documentation
If everything looks good, merge the pull request.

welcome · 2023-11-01T15:59:28Z

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

JasonWeill · 2023-11-01T19:47:02Z

@aws-khatria Thank you very much for circulating this pull request! Is this something that we should document in our user documentation? We already document some other options in the user docs. I can help with this if you need.

aws-khatria · 2023-11-01T20:12:42Z

@aws-khatria Thank you very much for circulating this pull request! Is this something that we should document in our user documentation? We already document some other options in the user docs. I can help with this if you need.

Sure, I'll update the PR with doc details. Let me know if there are any other comments as well.

3coins

@aws-khatria
Thanks for making this change. Can you rebase from main and fix the merge conflicts. Left some comments about using the model_parameters for the fields and api_keys. Let me know if you have questions.

packages/jupyter-ai/jupyter_ai/extension.py

packages/jupyter-ai/jupyter_ai/config_manager.py

dlqqq

Awesome work! There's a few changes that need to be done before this can be merged however.

packages/jupyter-ai/jupyter_ai/extension.py

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

aws-khatria

Addressed comments.

docs/source/users/index.md

JasonWeill

Generally looking OK; a couple of concerns about naming

packages/jupyter-ai/jupyter_ai/extension.py

3coins

@aws-khatria
Thanks for making all the updates, it looks like we are almost there. Left some naming suggestions, and a few questions.
I still have to test your changes locally, will be able to get to this by later this week; feel free to make the rest of the updates in the mean time.

packages/jupyter-ai/jupyter_ai/extension.py

docs/source/users/index.md

packages/jupyter-ai/jupyter_ai/config_manager.py

3coins · 2024-01-24T21:37:39Z

@aws-khatria
This branch seems to be out of date, can you rebase from latest main. Thanks.

3coins · 2024-01-24T22:33:48Z

@aws-khatria
I did some testing, and although the defaults seem to work when user doesn't have any values set, it breaks as soon as the user configures new values via the UI and restarts the server. For example,

I started with no defaults and set the model provider (OpenAI) and API keys through the UI, then configured the defaults as OpenAI, but did not provide the API key; this resulted in a crash, even though api keys were written to the config before the restart.
```
jupyter_ai.config_manager.AuthError: Missing API key for 'OPENAI_API_KEY' in the config.
```
Start with default provider as OpenAI, and added default api keys, but then changed the provider to anthropic in the UI. Restarted the server, and see that OpenAI was selected again as the provider.

This is due to incorrect merge as pointed in my comments. Rather than doing a shallow merge, which overwrites the top level values directly, we should be using the Merger.merge function to do deep merge, and also make sure precedence is given to the existing config. See an example for similar merge in ConfigManager.

merged_config = Merge.merge(default_config, existing_config)

aws-khatria · 2024-01-25T17:56:34Z

@3coins - I do agree with the deep merge. Somehow, it got messed up in merge conflict.

Although, I think default's must be given priority over what's present in the config file. Consider this scenario:

Start JL with no default
Select any Open AI model
Stop the server
Start JL with default as Claude. By default, Claude should be selected.

Enterprise might have use-case where they want a default language model but user can change it via UI. In this case, if the server restarts then enterprise would expect user to have the default selected model. If this is not the case, then I would suggest adding force_update parameter too that will control if the CLI provided input takes precedence over what exist in the config. Let me know your thoughts!

3coins · 2024-01-25T18:33:11Z

@aws-khatria
Thanks for explaining that use-case. My understanding was to apply default values when the user hasn't specified one. In the case you have mentioned, since the user selected OpenAI and it was saved as an option, system should honor user's choice, most users will expect that.

Changing the saved choice to the default on restart seems like an anti-pattern, a surprise for users that will need to be communicated appropriately to the user when the change happens. This also adds friction to users who will have to re-enter their choice of model on each restart, when they don't want default. My suggestion is to explore the force_update option in another issue/PR, so we can discuss pros/cons and different scenarios affecting it; this will also give us time to get feedback from the community.

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

welcome · 2024-01-26T03:44:23Z

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️

3coins · 2024-01-30T21:01:08Z

@meeseeksdev please backport to 1.x

Co-authored-by: aws-khatria <[email protected]>

* Setting default model providers * Setting default model providers * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Setting default model providers * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Added default_ prefix to new attributes * Added default_ prefix to docs as well * Fix docs * Removed redundant code * Incorporated naming related comments * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Removing unnecessary files * Corrected config names, simplified docs. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Piyush Jain <[email protected]>

* add failing test that asserts fields are included in lm_provider_params * fix lm_provider_params prop to include fields * fix bug that writes to `self.settings["model_parameters"]` * add test capturing bug introduced by #421 * pre-commit

JasonWeill added the enhancement New feature or request label Nov 1, 2023

3coins mentioned this pull request Dec 5, 2023

Remove config.json-related information #503

Merged

3coins reviewed Dec 14, 2023

View reviewed changes

packages/jupyter-ai/jupyter_ai/extension.py Outdated Show resolved Hide resolved

packages/jupyter-ai/jupyter_ai/config_manager.py Outdated Show resolved Hide resolved

dlqqq requested changes Dec 18, 2023

View reviewed changes

packages/jupyter-ai/jupyter_ai/extension.py Outdated Show resolved Hide resolved

packages/jupyter-ai/jupyter_ai/extension.py Outdated Show resolved Hide resolved

aws-khatria closed this Jan 23, 2024

aws-khatria force-pushed the default_model_provider_v1 branch from 8608b1e to b2e7c95 Compare January 23, 2024 04:58

aws-khatria added 2 commits January 22, 2024 21:08

Setting default model providers

752e169

Setting default model providers

d198e11

aws-khatria reopened this Jan 23, 2024

pre-commit-ci bot and others added 7 commits January 23, 2024 05:34

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

8b1924e

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

Setting default model providers

8ab53f3

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

f514ea4

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

Added default_ prefix to new attributes

09a49b8

Added default_ prefix to docs as well

00f23aa

Fix docs

bad649a

Removed redundant code

ff73fac

aws-khatria commented Jan 23, 2024

View reviewed changes

JasonWeill reviewed Jan 23, 2024

View reviewed changes

docs/source/users/index.md Outdated Show resolved Hide resolved

JasonWeill reviewed Jan 23, 2024

View reviewed changes

packages/jupyter-ai/jupyter_ai/extension.py Outdated Show resolved Hide resolved

packages/jupyter-ai/jupyter_ai/extension.py Outdated Show resolved Hide resolved

3coins requested changes Jan 24, 2024

View reviewed changes

aws-khatria and others added 3 commits January 25, 2024 14:57

Incorporated naming related comments

4d57c31

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

d5d4502

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

Removing unnecessary files

786a9bf

aws-khatria and others added 2 commits January 25, 2024 15:01

Merge branch 'jupyterlab:main' into default_model_provider_v1

e7cec53

Corrected config names, simplified docs.

50e038b

3coins approved these changes Jan 26, 2024

View reviewed changes

3coins merged commit 84c074a into jupyterlab:main Jan 26, 2024
8 checks passed

meeseeksmachine pushed a commit to meeseeksmachine/jupyter-ai that referenced this pull request Jan 30, 2024

Backport PR jupyterlab#421: Setting default model providers

48097b1

meeseeksmachine mentioned this pull request Jan 30, 2024

Backport PR #421 on branch 1.x (Setting default model providers) #608

Merged

3coins pushed a commit that referenced this pull request Jan 30, 2024

Backport PR #421: Setting default model providers (#608)

0246114

Co-authored-by: aws-khatria <[email protected]>

dlqqq added a commit that referenced this pull request Nov 27, 2024

add test capturing bug introduced by #421

7b427df

dlqqq mentioned this pull request Nov 27, 2024

Update model fields immediately on save #1125

Merged

dlqqq added a commit that referenced this pull request Dec 2, 2024

add test capturing bug introduced by #421

b8d65cd

dlqqq added a commit that referenced this pull request Dec 2, 2024

add test capturing bug introduced by #421

f82785e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setting default model providers #421

Setting default model providers #421

aws-khatria commented Nov 1, 2023 •

edited by JasonWeill

Loading

welcome bot commented Nov 1, 2023

JasonWeill commented Nov 1, 2023

aws-khatria commented Nov 1, 2023

3coins left a comment

dlqqq left a comment

aws-khatria left a comment

JasonWeill left a comment

3coins left a comment

3coins commented Jan 24, 2024

3coins commented Jan 24, 2024

aws-khatria commented Jan 25, 2024

3coins commented Jan 25, 2024 •

edited

Loading

welcome bot commented Jan 26, 2024

3coins commented Jan 30, 2024

Setting default model providers #421

Setting default model providers #421

Conversation

aws-khatria commented Nov 1, 2023 • edited by JasonWeill Loading

welcome bot commented Nov 1, 2023

JasonWeill commented Nov 1, 2023

aws-khatria commented Nov 1, 2023

3coins left a comment

Choose a reason for hiding this comment

dlqqq left a comment

Choose a reason for hiding this comment

aws-khatria left a comment

Choose a reason for hiding this comment

JasonWeill left a comment

Choose a reason for hiding this comment

3coins left a comment

Choose a reason for hiding this comment

3coins commented Jan 24, 2024

3coins commented Jan 24, 2024

aws-khatria commented Jan 25, 2024

3coins commented Jan 25, 2024 • edited Loading

welcome bot commented Jan 26, 2024

3coins commented Jan 30, 2024

aws-khatria commented Nov 1, 2023 •

edited by JasonWeill

Loading

3coins commented Jan 25, 2024 •

edited

Loading