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

chore: dynamic plugin imports #1383

Open
wants to merge 23 commits into
base: develop
Choose a base branch
from

Conversation

ChristopherTrimboli
Copy link
Contributor

Relates to:

None, just looking for improvements.

Risks

HIGH - could break plugins

Background

Plugins even if unused are being always imported into the agent/index.ts at top of file.
This adds much memory usage to the JS runtime and not most efficient way to load optional plugins.

What does this PR do?

Dynamically imports plugins when needed based on ENV / secrets loaded.

What kind of change is this?

Improvements (misc. changes to existing features)

Why are we doing this? Any context or related work?

Makes Eliza faster / slimmer / less bloated / scales infinite plugins with minimal runtime overhead.

Documentation changes needed?

My changes do not require a change to the project documentation.

Testing

Try out different combinations of plugins off and on, see if loads or breaks.

Discord username

cjft

@ChristopherTrimboli
Copy link
Contributor Author

P.S. - seems like passed all tests, omg, nice... just noticed could do same for all client packages: if we like this change, ill do another PR for dynamic client imports too, massive optz.

@ChristopherTrimboli
Copy link
Contributor Author

ChristopherTrimboli commented Dec 22, 2024

hmmm avaer told me that "alot of wrappings" I can make better abstraction for loading dynamic imports... I'll work on. Like a function for it instead. Bit complex because of all the diff conditionals / import styles / 1 is a default import. I'll see if can do nicely.

@ChristopherTrimboli
Copy link
Contributor Author

hmmm avaer told me that "alot of wrappings" I can make better abstraction for loading dynamic imports... I'll work on. Like a function for it instead. Bit complex because of all the diff conditionals / import styles / 1 is a default import. I'll see if can do nicely.

done, now have loadPlugin() abstraction with very simple string based plugin config.

@ryanleecode
Copy link
Contributor

ryanleecode commented Dec 23, 2024

Why do it like this? Why not pass a callback to loadPlugin that is async () => import('my plugin')

@ryanleecode
Copy link
Contributor

The probably is the public api is purely string base its not type safe where as if you do callback import and you don't have the dependency installed, typescript will not compile it.

@odilitime
Copy link
Collaborator

Probably need to update

* 5. Edit the 'index.ts' file in 'agent/src': *

for future plugin devs

@ChristopherTrimboli
Copy link
Contributor Author

ChristopherTrimboli commented Dec 23, 2024

Why do it like this? Why not pass a callback to loadPlugin that is async () => import('my plugin')

OK yeah callback param is better, I'll update.

@ChristopherTrimboli
Copy link
Contributor Author

changed to callback params: 2efee64

@ChristopherTrimboli
Copy link
Contributor Author

Probably need to update

* 5. Edit the 'index.ts' file in 'agent/src': *

for future plugin devs

c5d4dc4 updated plugin instructions

@ryanleecode
Copy link
Contributor

This doesn't work your loadPlugin returns null but the function is typed as Promise<any> where as the constructor parameter only takes Plugin[]. Giving it a null plugin will definately fail.

IMO the correct approach would be something as follows.

  1. Create a dynamic Plugin type such as
export type DynamicPlugin = {
    readonly factory: () => Promise<Plugin>;
    readonly shouldInit: (character: Character) => boolean;
};
  1. Make the AgenRuntime constructor private and create a static factory function that is async and accepts ReadonlyArray<Plugin | DynamicPlugin> as a parameter. Await the dynamic parameters in the async static factory. Bonus points for adding _tag: 'Plugin' and _tag: 'DynamicPlugin to the respective types, although this would be a high impact change since it would fuck over downstream plugins.

@ChristopherTrimboli
Copy link
Contributor Author

This doesn't work your loadPlugin returns null but the function is typed as Promise<any> where as the constructor parameter only takes Plugin[]. Giving it a null plugin will definately fail.

IMO the correct approach would be something as follows.

  1. Create a dynamic Plugin type such as
export type DynamicPlugin = {
    readonly factory: () => Promise<Plugin>;
    readonly shouldInit: (character: Character) => boolean;
};
  1. Make the AgenRuntime constructor private and create a static factory function that is async and accepts ReadonlyArray<Plugin | DynamicPlugin> as a parameter. Await the dynamic parameters in the async static factory. Bonus points for adding _tag: 'Plugin' and _tag: 'DynamicPlugin to the respective types, although this would be a high impact change since it would fuck over downstream plugins.

I see, more complex with types then I originally thought, will think about / work on, thanks for feedback.

@ryanleecode
Copy link
Contributor

This doesn't work your loadPlugin returns null but the function is typed as Promise<any> where as the constructor parameter only takes Plugin[]. Giving it a null plugin will definately fail.
IMO the correct approach would be something as follows.

  1. Create a dynamic Plugin type such as
export type DynamicPlugin = {
    readonly factory: () => Promise<Plugin>;
    readonly shouldInit: (character: Character) => boolean;
};
  1. Make the AgenRuntime constructor private and create a static factory function that is async and accepts ReadonlyArray<Plugin | DynamicPlugin> as a parameter. Await the dynamic parameters in the async static factory. Bonus points for adding _tag: 'Plugin' and _tag: 'DynamicPlugin to the respective types, although this would be a high impact change since it would fuck over downstream plugins.

I see, more complex with types then I originally thought, will think about / work on, thanks for feedback.

Actually I stand mistaken since there is a filter(Boolean) and the end the nulls get filtered out. So this would work but I don't think we need the third parameter b/c u can do import("whatever").then((mod) => mod.myPlugin) which is also type safe.

@ryanleecode
Copy link
Contributor

Few more comments.

  1. probably export the loadPlugin function
  2. create another function loadPlugins, and then i can just spread the resulting array

(found issues in Plugin types)
@ChristopherTrimboli
Copy link
Contributor Author

OK strictly type loadPlugins with Plugin + mod style, this works and is type compliant however we now have 3 plugins not correctly typed to Plugin that were hidden found:

@elizaos/plugin-solana
@elizaos/plugin-near
@elizaos/plugin-goat

image

This is bigger PR to fix all plugins now, was never type safe to begin with tbh.

@ryanleecode
Copy link
Contributor

Have you run pnpm build in the root of the monorepo?

@ChristopherTrimboli
Copy link
Contributor Author

Have you run pnpm build in the root of the monorepo?

ah, fixed, OK everything compiling / working great. Gonna work on:

1.) export the loadPlugin function
2.) create another function loadPlugins, and then i can just spread the resulting array

to cleanup and ready for review / testing. thanks for help! TS god

agent/src/index.ts Outdated Show resolved Hide resolved
@ChristopherTrimboli
Copy link
Contributor Author

I went big boy AAA on this one:

23cb080

I think this proper now. ready for review kind citizen.

packages/core/src/plugins.ts Outdated Show resolved Hide resolved
packages/core/src/plugins.ts Outdated Show resolved Hide resolved
packages/core/src/plugins.ts Outdated Show resolved Hide resolved
Comment on lines 468 to 490
importFn: () =>
import("@elizaos/plugin-coinbase").then(
(m) => m.coinbaseMassPaymentsPlugin
),
},
{
secrets: ["COINBASE_API_KEY", "COINBASE_PRIVATE_KEY"],
importFn: () =>
import("@elizaos/plugin-coinbase").then((m) => m.tradePlugin),
},
{
secrets: ["COINBASE_API_KEY", "COINBASE_PRIVATE_KEY"],
importFn: () =>
import("@elizaos/plugin-coinbase").then(
(m) => m.tokenContractPlugin
),
},
{
secrets: ["COINBASE_API_KEY", "COINBASE_PRIVATE_KEY"],
importFn: () =>
import("@elizaos/plugin-coinbase").then(
(m) => m.advancedTradePlugin
),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking a helper function (my idea of load plugins) which is just import('@elizaos/plugin-coinbase').then((mod) => [mod.pluginA, modPluginB, modPluginC]). Then we don't need to import coinbase 3 times.

Copy link
Contributor

@ryanleecode ryanleecode Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And then to add them you can just spread:

plugins: [
...myCoinbasePluginsDynamicallyImportedOrNull
].filter(Boolean)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok this final point I think, taking break, will work on that, we getting close 👍🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

solved importCache I believe: 32c7e5a

Main issue was handling unique secrets, I do like the simple statements of secrets and importFn, now if duplicate importFn, it uses a cache, so won't import duplicates, but DX is much nicer else would have big nested ternaries and if/else stuffed in array with secrets spread.

If have better style lemme know.

Copy link
Contributor Author

@ChristopherTrimboli ChristopherTrimboli Dec 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case that secrets are exact same, uses your suggestion, as array to import: 430dab1

But case still stands I think best separated if secrets are unique, for webhook, and others.
https://github.com/elizaOS/eliza/pull/1383/files#diff-935219608f7b5ca6c8b8548cfdce88c7d3cdb6bb6d9f9d8df644b364f6557e4eR437-R462

agent/src/index.ts Outdated Show resolved Hide resolved
agent/src/index.ts Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants