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

Config V2 #353

Merged
merged 18 commits into from
Sep 7, 2023
Merged

Config V2 #353

merged 18 commits into from
Sep 7, 2023

Conversation

dlqqq
Copy link
Member

@dlqqq dlqqq commented Aug 19, 2023

Description

This is a significant refactor of how Jupyter AI handles user configuration. Our previous approach was implemented mainly in the interest of time. This new config implementation should be significantly more robust, with fewer edge-cases. In addition, the developer APIs should also be much more readable.

Demo

Screen.Recording.2023-09-04.at.5.57.16.PM.mov

Backend changes

  • The ConfigManager class (CM) has been completely rewritten from scratch.
  • CM now uses filesystem mtime info to cache disk reads of the config file.
  • CM automatically formats the config file to a human-readable format (with indentation) by default. This should now happen automatically when the server is started.
  • CM no longer returns the values of previously set API keys as a security measure. In other words, the API keys can only be updated or deleted now, but never read from the REST API.
  • ConfigManager.update_config() is used to create and update user config, including API keys. In addition, this method now accepts a partial object, rather than requiring a full copy of the user config from the client.
  • ConfigManager.delete_api_key(key_name: str) is a new method used to delete API keys. There is now a corresponding REST API exposing this method.
  • When the config is updated, CM now validates the config file against a JSON schema. Later on, we may add new metadata to the default JSON schema to offer additional validation options.
  • ConfigManager is now traitlets-configurable, with 3 traits:
    • config_path: path to the user's config file
    • schema_path: path to the user's schema file, i.e. the JSON schema that is used to validate the config file. If this does not exist, CM will automatically copy our default schema to this path.
    • indentation_depth: how many spaces to use to format the config file. Defaults to 4.
  • Adds comprehensive Python unit test coverage for the CM. All methods and filesystem side effects are tested.

Frontend changes

  • Mostly refactoring with a few new components (under components/mui-extras) to add some quality-of-life to the Settings form.
  • New UI for editing and deleting API keys, with better error handling and validation.
  • Both the backend and the UI implement a "last read" check which, on save, validates that the file was not written to disk after the user first opened the Settings form. This ensures that no write-write conflicts occur in multi-user scenarios.
  • Greatly improved UI for indicating success/error states when submitting the Settings form.
  • Slightly reduced top and side padding in the Settings form to match UI of other sidebar widgets.
  • Adds JS unit test coverage for the partial update logic.

Follow-up issues

  • Icon button tooltips in the API keys section
    • This should be a new component, since it's very verbose to implement this out-of-the-box for each icon button.
  • Frontend field validation
    • We currently don't validate most text fields on the frontend. Right now, most validation is handled on the backend. As this is existing behavior, I chose to omit this feature from this PR in the interest of getting this merged more quickly.
  • Add Python and JS unit test workflows
  • Client-side API key validation: Disable "delete" button on API keys in use by currently selected providers.

@dlqqq dlqqq force-pushed the config branch 2 times, most recently from 100e7fb to 603ea16 Compare August 19, 2023 21:07
@dlqqq dlqqq added the enhancement New feature or request label Aug 19, 2023
@krassowski
Copy link
Member

I see that this is a draft but given no description/references I just wanted to ask if the intention is to address #313 too, or if I shall open a PR to implement it. If you are open to PRs, I suspect it makes sense to wait until this one is merged to avoid conflicts/rewrite. Is there a rough timeline for this?

@dlqqq dlqqq force-pushed the config branch 2 times, most recently from 7c08996 to d40f215 Compare September 5, 2023 00:56
@dlqqq dlqqq marked this pull request as ready for review September 5, 2023 01:03
@dlqqq
Copy link
Member Author

dlqqq commented Sep 5, 2023

@krassowski Thanks for following up about #313! This PR doesn't address #313, but that can be implemented immediately after this PR is merged. I just marked this PR ready for review, so this should be in by the end of the week. If you'd like, we can assign #313 to you if you'd like to own the issue after this PR is merged.

For context, I was violently ill the last week, so this PR had taken much longer than I had initially hoped. Thankfully, I feel better now, so any changes to the config should be unblocked very shortly. 😁

@krassowski
Copy link
Member

Thank you for getting back to me. I hope you feel good now! I assigned myself and will start working on it on top of this PR.

@JasonWeill
Copy link
Collaborator

If we're making a developer API available for config, should it go into a new Developers section of the Jupyter AI docs?

@dlqqq dlqqq merged commit fc711c7 into jupyterlab:main Sep 7, 2023
5 checks passed
@dlqqq dlqqq mentioned this pull request Sep 7, 2023
dbelgrod pushed a commit to dbelgrod/jupyter-ai that referenced this pull request Jun 10, 2024
* config manager initial draft

* implement basic client for new config manager

* implement JL4 unit testing (do not backport)

* implement chat settings save

* implement client API key deletion

* greatly improve error banner handling in settings

* implement API key edit and delete in settings UI

* ensure API keys are not empty

* reduce top and side padding in settings UI

* pre-commit

* fix integer field type error from rebase

* fix api key edit/delete react bugs

* improve UI error formatting

* implement last_read check on settings form

* fix local model ID not showing

* remove outdated comment

* ensure no empty field dictionaries in config

* fix minor typo
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
3 participants