Quansight / ragna

RAG orchestration framework ⛵️
https://ragna.chat
BSD 3-Clause "New" or "Revised" License
179 stars 22 forks source link

[ENH] - Support Entry Points for Plugins #189

Open nenb opened 1 year ago

nenb commented 1 year ago

Feature description

Add support for Entry Points for Plugins to allow easy code-sharing eg for a new assistant in #173. Also some details in #166.

Value and/or benefit

Extensions can be easily shared - it's just a simple pip install. The current behaviour requires users to 'discover' an extension for themselves, and then to make sure that they install it correctly so that it's recognised by ragna.

Anything else?

This has been used very successfully (IMO) by llm.

Most of the hard work has already been done (ie the plug-in interfaces like ApiAssistant already exist), there is just the final-step of adding some packagaing details and some brief logic related to plug-in discovery.

I can mock-up a quick example if this is of interest.

pmeier commented 1 year ago

In general, I'm in favor of this. Like you said, it makes sharing really easy. One question arises though: where do we hook in external components? My current idea would be to do it as part of the component selection in the config wizard. We could just chain them to the builtin ones

https://github.com/Quansight/ragna/blob/dc497ef4768b8f8bd4d5c3736eb1d67b27050387/ragna/_cli/config.py#L161-L165

Of course we need to change the phrasing in the wizard to account for that. Plus, we probably need to put the class path as part of the selection (as well as ragna check) to make it clear where a component is coming from.

nenb commented 1 year ago

I just spent a little time on this and made an attempt with the pydantic2 model_post_init hook. I was reading through some of the comments in the code, and I realised that there are lots of ways for users to interact with the config settings (env vars, kwargs, TOML files etc). I settled on model_post_init as it felt like the most robust way to load the extensions. But perhaps it causes issues in other ways (and I think it does rely on pydantic version 2) - what is your opinion?

I also added a mock package and a notebook to show it works. All available in my branch here.

pmeier commented 1 year ago

I settled on model_post_init as it felt like the most robust way to load the extensions.

Could you explain how you got to that conclusion? I feel it would better fit in the config wizard as pointed out above for two reasons:

  1. For the Python API, you don't need to have the components as part of the config to use them. Meaning, you can just do

    from ragna import Config
    from vikings.assistants import Ivar
    
    Ivar(Config()).answer("Who are you?", [])

    IMO that is simpler and more expressive than doing what you have suggested in your notebook:

    from ragna import Config
    
    config = Config()
    config.core.assistants[1](config).answer("Who are you?", [])

    Here I don't know what assistant I'm getting, since the order is dependent on what packages are installed that use the entrypoint.

  2. It would be nice to be able to select the third-party components through the same config wizard that we use for the builtin components. With your hook on the config, this happens magically without the user knowing about it. And that ties again into our discussion in #180.
nenb commented 1 year ago

I think that I may only have a partial understanding of the intended workflows. I'm going to be more explicit here about my current understanding. Please correct the bits that I have wrong (and then I will follow-up with a response to your previous questions):

Workflow 1 - UI entrypoint (ragna ui) a. No TOML file is present. In this case, the program will exit and instruct to use the wizard. The user will use the wizard (ragna init) to generate the TOML and then return to the start. b. A valid TOML file is present. Proceed as usual. No wizard is called.

Workflow 2 - CLI entrypoint (ragna api) (same as workflow 1)

Worfklow 3 - Python API a. No TOML file is present. The user can rely on the pydantic defaults, or environment variables, or passing something explicitly to the constructor. No wizard is called. b. A valid TOML file is present. The user can rely on the pydantic defaults, or environment variables, or passing something explicitly to the constructor, or the TOML file. No wizard is called.

From my current understanding, the wizard is only called in situations 1.a and 2.a. Do I have this correct?

nenb commented 1 year ago

Could you explain how you got to that conclusion?

It seemed to me like a user could be relying on a TOML file, or environment variables, or passing something to the constructor, or the pydantic defaults for configuration. The simplest way that I could think of for loading the plugins in a way that didn't mess with the user's original config was to add them at the end with the post_init hook.

IMO that is simpler and more expressive than doing what you have suggested in your notebook:

My previous example was only to illustrate that the plugin was present in the config. I admit that it was a pretty ugly example to post in the notebook, and I should have thought about it more. I agree with your suggestion for the Python API syntax. This is also possible with the code on my branch (I pushed a commit a while ago that illustrates this).

I feel it would better fit in the config wizard

My current understanding (see my previous comment) is that there are many situations where the wizard will not be called eg the user wants to test a new plugin, and already has a valid TOML file. I wasn't sure how to deal with this situation. That was the main reason I didn't put it in the wizard.

With your hook on the config, this happens magically without the user knowing about it. And that ties again into our discussion in https://github.com/Quansight/ragna/issues/180.

I think I've taken up too much of your time now on the wizard - apologies! I agree that the best use of time now is probably for me to embrace it! With that personal hurdle out of the way, the only issue I see is how to deal with workflows that need a plugin and where the wizard is not involved (see above eg using the Python API). What would be your suggestion for handling plugins in these cases?

Once I have your ideas on this, I'll update the branch to include everything you suggest.

pmeier commented 1 year ago

From my current understanding, the wizard is only called in situations 1.a and 2.a. Do I have this correct?

Correct.

My current understanding (see my previous comment) is that there are many situations where the wizard will not be called eg the user wants to test a new plugin, and already has a valid TOML file. I wasn't sure how to deal with this situation. That was the main reason I didn't put it in the wizard.

My point in this discussion here and in #180 as well is that components have requirements and users need an easy way to discover them. pip installing any package just gives them access to the code they need, but won't do anything for the components to actually be available. And that is the main concern. Unless you find a way to educate the users on how to meet the requirements that doesn't involve the wizard, the wizard stays and all config generation will be done with it.

many situations where the wizard will not be called eg the user wants to test a new plugin, and already has a valid TOML file.

The point of the configuration file is to be explicit. Imagine you needing to deploy this and don't know about the magic plugin handling. For some reason, the apps keep crashing on startup with an error message that points to a component that you haven't even selected as part of your config. This is horrible UX. I would much rather have users add a few extra lines to the configuration file to avoid such a situation.

One thing that we can do is to add an --update flag to ragna init that re-runs the wizard, but uses an existing file as base rather than creating one from scratch. So if users do something like pip install vikings, they will either need to fix their existing configuration file by hand or run ragna init --update and select the new components like that.

nenb commented 1 year ago

I've added an attempt to incorporate it into the wizard here. What do you think? (I put it in 'common' rather than 'builtin' because I felt that a plugin wasn't a builtin. I can change it if preferred.)

One thing that we can do is to add an --update flag to ragna init that re-runs the wizard, but uses an existing file as base rather than creating one from scratch.

Seems reasonable. Is this a separate issue, or would I add it to the my current branch?

The point of the configuration file is to be explicit.

One comment on this - it seems like env vars currently override the config file. Is this consistent with this approach? It could be disabled via pydantic (but perhaps it's considered a necessary exception).

pmeier commented 1 year ago

What do you think?

My preferred way of doing it would be as I have described it in https://github.com/Quansight/ragna/issues/189#issuecomment-1803931462. Let _select_components take another argument for an entrypoint name and just chain the third party components together with the builtin ones to have all of them in the same dialog. To avoid naming conflicts, we should "namespace" the options based on the module the come from. SO maybe something like

Is this a separate issue

Separate issue. While it helps the adoption of the entrypoints, this is a change that can stand on its own. Maybe I just want to make a change to an existing configuration file and want to have the proper guidance.

it seems like env vars currently override the config file. Is this consistent with this approach?

Yes, this is consistent, because env vars are set by the person deploying this and thus are explicit as well. Plus, we need this kind of behavior, since something like an URL could very easily change between different deployments, but you don't want to have a different configuration file each time.

pmeier commented 1 year ago

All of that being said, I'm not sure if I would merge a PR for this right away: there already exists a way for users to hook in third-party components. This would only make it easier. While this is a good thing, we should actually wait for demand before we go for the change. Meaning, unless there is a third-party library that says "hey, we want these hooks" this change only makes it more complicated for us without any benefit.

nenb commented 1 year ago

No problem. Happy for you to close/leave open this issue as you see fit.

I'm not sure if it's useful to archive my current branch in some way eg open a PR and then immediately close it, in case there is any desire to refer back to it in the future?

pmeier commented 1 year ago

If you are up to that, I think sending a PR linking this thread would be fine. Right now our issue / PR tracker is reasonably small that we don't need to close stuff to avoid losing track.