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 to disable providers using traitlets config #313

Closed
krassowski opened this issue Aug 2, 2023 · 7 comments · Fixed by #415
Closed

Allow to disable providers using traitlets config #313

krassowski opened this issue Aug 2, 2023 · 7 comments · Fixed by #415
Assignees
Labels
enhancement New feature or request project:config Configuration mechanisms

Comments

@krassowski
Copy link
Member

Problem

  • A lot of jupyter configuration happens on the traitlets level (e.g. jupyter_server_config.py or .json files), providing a standardisation across projects and extensions with know format, location and runtime type checking
  • I would like to disable specific model providers (and enable others) in jupyter-ai using the traitlets

Currently the providers are enabled by entrypoints which is convenient for extending but not to for configuring; it is much more labour intensive to disable an entry point, and disabling all entrypoints which are shipped today does not guarantee that new providers added in future versions will be disabled too.

Proposed Solution

  1. Add allow/block list extension configuration options:
    • AiExtension.allowed_providers: List[str]
    • AiExtension.blocked_providers: List[str]
  2. Make ModelProviderHandler and friends respect these settings
  3. (Optionally, but preferably) use black magic to disable entrypoints from kicking in depending on these settings
@krassowski krassowski added the enhancement New feature or request label Aug 2, 2023
@dlqqq
Copy link
Member

dlqqq commented Aug 2, 2023

@krassowski We are also inclined to add this feature, but were thinking it would be implemented in our current configuration system under $JUPYTER_DATA_DIR/jupyter_ai/config.json. I'm not a big fan of traitlets personally; making a class that inherits from another also be traitlets-configurable requires multiple inheritance and declaring the metaclass. Furthermore, AFAIK, traitlets configuration is not mutable at runtime, which means that operators would have to reboot the server to alter configuration. Are there any features you want to see that are exclusive to traitlets? If not, I'm inclined to implement this under our in-house configuration system rather than via traitlets.

@krassowski
Copy link
Member Author

krassowski commented Aug 2, 2023

Thank you for a super quick reply!

I'm not a big fan of traitlets personally; making a class that inherits from another also be traitlets-configurable requires multiple inheritance and declaring the metaclass

I thought you already do?

> type(AiExtension)
traitlets.traitlets.MetaHasTraits

I understand that getting familiar with traitlets may have been a steep curve, and those are not always the best solution but I don't yet see how those would be a problem here specifically. I of course respect that you may prefer the other way for consistency, though again users may prefer traitlets. I don't think it is a strong preference for users though - having it either way would be better than not having it at all :)

AFAIK, traitlets configuration is not mutable at runtime, which means that operators would have to reboot the server to alter configuration

Depends on what you mean by mutable. In a sense of hot reloading? Right. In a sense of being immutable - nope (this is how IPython runtime reconfiguration works) - so it would depend on how changes are made.

Are there any features you want to see that are exclusive to traitlets?

Kind of. If we also wanted to have AiExtension.extra_providers: List[Union[str, BaseProvider]] (as complementary to entrypoints), this cannot be done in a json file. One could also consider a hardening step in which AiExtension.allowed_providers is taking a list of instances rather than string identifiers, as string identifiers could be overridden by entry points (though if you don't trust your entry points, why would you trust your instance integrity).

What do you think?

@krassowski
Copy link
Member Author

krassowski commented Aug 2, 2023

Just for reference, #229 implements a traitlet handling mechanism that could help here. Also related to issue #218.

@dlqqq
Copy link
Member

dlqqq commented Aug 2, 2023

@krassowski

I thought you already do?

We're not importing it directly, we inherit traitlets configurability from jupyter_server.extension.application.ExtensionApp. You can see here that traitlets is not used at all in our main code, only as a comment in the cookiecutter (currently on dev hiatus):

Screenshot 2023-08-02 at 10 05 26 AM

Right. In a sense of being immutable - nope (this is how IPython runtime reconfiguration works) - so it would depend on how changes are made.

Sorry, could you elaborate more on this? If a user sets some configuration via traitlets like AiExtension.allowed_providers=["openai"], is there a way for that user to change the configuration at runtime? I understand that in the application logic, our code can still change the value of the trait dynamically as much as we'd like, but is there a built-in method for the user to alter their original configuration at runtime?

string identifiers could be overridden by entry points

We're not using the entry point name as an ID per-provider, we already have an id field defined on each provider class, and we throw a runtime error if you try to use two providers with the same id. So I don't think it's possible to override a provider ID via entry points.

All things considered, I think keeping all our configuration in a unified file path makes more sense. Having two different configuration APIs that differ in runtime-configurability for operators can be confusing. I acknowledge that implementing this without traitlets is more difficult for us, but I do believe the pros outweigh the cons.

@krassowski
Copy link
Member Author

We're not importing it directly, we inherit traitlets configurability from jupyter_server.extension.application.ExtensionApp. You can see here that traitlets is not used at all in our main code, only as a comment in the cookiecutter

But this shows that existing integration is friction less and does not require any problematic meta classes patterns, right?

Sorry, could you elaborate more on this?

Sure! Traitlets implement the observer pattern which is used in IPython to do this kind of reconfiguration. User could just import AiExtension instance and set a property and a good implementation of traitlets would update state as needed. In IPython this can be done directly on instances, or by using %config magic, please see Introduction to IPython configuration

We're not using the entry point name as an ID per-provider, we already have an id field defined on each provider class, and we throw a runtime error if you try to use two providers with the same id.

That's great!

@ellisonbg
Copy link
Contributor

@krassowski thanks for opening this issue, some of these ideas have been bouncing around in my head, but we hadn't talked about it yet. Very helpful! A few comments:

  • I think there are two levels of config here, that somewhat collapse into a single level for individual users. However, for entities that are deploying Jupyter and Jupyter AI to a large number of users, those two levels are serve very different needs. Because of that, I will focus my comments on the large scale usage of Jupyter AI, where there is one group that deploys and operates Jupyter to a group of end users.
  • The operators will likely have strong constraints on which models or providers their users can access and how the auth is wired up. They will likely want to avoid having all their users managing individual level API keys, etc. For these admin operations, I believe @krassowski is spot on. It should be a traditional Jupyter config system, based on traitlets, located in the standard config directories and serve to constraint what the end users can see and do.
  • For the end user in these use cases our UI is less about config and more about choosing which model you want to use right now. Given that, and the fact that it needs to be runtime mutable, I think this data should not be stored in config directories, but in the runtime data directores. While traitlets does allow runtime mutations, we can't persist those mutations in config files in the standard config directories (an Admin will likely make this read only).
  • It isn't clear to me if Admins will want to set policies for providers, for models, or for both.
  • We should leave the door open an Admin also constraining how auth is done for a given model. They may want to allow access to a give model+provider, but want to now let the user specify the API KEY (instead, provide it in the config).

@dlqqq
Copy link
Member

dlqqq commented Aug 10, 2023

@ellisonbg Thanks for your input. I see your perspective and how this configuration doesn't need to be dynamic. If that's the case, then indeed we can use the traitlets approach as originally suggested by @krassowski.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request project:config Configuration mechanisms
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants