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

Update model fields immediately on save #1125

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Update model fields immediately on save #1125

wants to merge 5 commits into from

Conversation

dlqqq
Copy link
Member

@dlqqq dlqqq commented Nov 27, 2024

Description

Fixes usage of model fields by respecting the updated values immediately on save (without requiring a restart).

Model fields are used to specify provider-specific keyword arguments directly from Jupyter AI's Settings UI. These include Base API URL for OpenAI, Region name for Amazon Bedrock, etc.

Demo

Screen.Recording.2024-11-27.at.10.33.12.AM.mov

Details for contributors

  • Fixes a bug introduced by Setting default model providers #421: BaseProvider.get_model_parameters() would return the fields set by the existing config.json on init, instead of the user-specified overrides in the AiExtension.model_parameters trait.

    • This was due to the ConfigManager accidentally merging the existing config into self.settings["model_parameters"] instead of a deep copy of that dictionary.
  • Fixes a bug introduced by Distinguish between completion and chat models #711: Model fields were not being returned at all from the dictionary returned by ConfigManager.lm_provider_params.

  • Makes the allowed_providers, blocked_providers, allowed_models, blocked_models arguments to ConfigManager optional and default to None, as indicated by their type signatures.

    • This change will not have any impact on users, as Jupyter AI always provides these arguments on extension init. This is purely for the convenience of contributors authoring tests.
  • Adds unit test coverage for both aforementioned bugs.

Testing instructions

  • Make the following changes in config.py:
    • Change deepcopy(self._defaults.get(config_key)) to self._defaults.get(config_key) near line 240.
    • Remove the line **fields, near line 475.
  • Run pytest, and verify that the 2 added tests fail. This asserts that the tests actually capture the bugs introduced by Setting default model providers #421 and Distinguish between completion and chat models #711.
  • Revert the changes and re-run pytest, and verify the 2 added tests now pass.
  • Run Jupyter AI from this branch, and verify that model fields are updated immediately on save.

@dlqqq dlqqq added the bug Something isn't working label Nov 27, 2024
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.

  1. Tested the deepcopy change by removing the code for it and failing pytest. Reinstating it works as intended.
  2. Tested that the changes to model fields are saved and there is no need to restart Jupyter AI. The system logs report the change as intended. Resolves Issue Provider text fields values saved from Chat UI are only applied after Jupyter Lab is restarted. #1118 .
  3. Issue profile_name field ignored for Amazon Bedrock Chat Models in jupyter-ai Extension #1007 : used blank profile_name and new region and the saved model works well.
  4. Issue Missing Request Schema for SageMaker Endpoint in jupyter-ai Extension #1006 (SageMaker endpoint request schema error). This still needs to be tried though there may not be JAI users who are calling SageMaker for the models.
  5. Issue api_version in GUI not considered, need to set OPENAI_API_VERSION #807 to be tested but no access to Azure. Hopefully @OliverKleinBST can test this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants