Ian/mastra slim #2972
Ian/mastra slim #2972
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
|
@ianmacartney is attempting to deploy a commit to the Mastra Team on Vercel. A member of the Team first needs to authorize it. |
|
📝 Documentation updates detected! You can review documentation updates here |
There was a problem hiding this comment.
Thank you for this Pull Request. This is not an easy feat to grasp, but you did a great job!
The things I would really want to keep are the following parts:
- Betters async/await in proxy storage, instead of my setup function
- vector proxy implementation
I've commented on the issue what my strategy is to get what we all want.
| } else { | ||
| this.storage = new Promise((resolve, reject) => { | ||
| try { | ||
| import(['./', 'libsql'].join('')) // avoid automatic bundling |
There was a problem hiding this comment.
Sadly, this can quickly become a foot gun, where we ignore optimizing these files. If we ever introduce a side effect, our tooling will not capture it.
There was a problem hiding this comment.
Without this, the challenge I ran into is that esbuild will automatically analyze ./libsql and run into this error: Cannot find module '@libsql/linux-arm64-gnu'. I'm not sure the best way to do this. I was considering pushing the try/catch import down into ./libsql/index.ts for @libsql/client entirely, but the exported class depends on the base class.. any other ideas?
The problem ultimately is that if we do the regular string import, @libsql/client can't be an optional dependency and might not get stripped during bundling (though I'm not an expert at this)
packages/core/src/memory/memory.ts
Outdated
| `Found deprecated Memory vector db file ${oldDb} this db is now merged with the default ${newDb} file. Delete the old one to use the new one. You will need to migrate any data if that's important to you. For now the deprecated path will be used but in a future breaking change we will only use the new db file path.`, | ||
| ); | ||
| const hasNewDb = existsSync(join(process.cwd(), newDb)) || existsSync(join(process.cwd(), '.mastra', newDb)); | ||
| const connectionUrl = |
There was a problem hiding this comment.
Same as above, we should put these env pieces in mastra
There was a problem hiding this comment.
This isn't imported or used from Mastra, and didn't previously exist - it seems to only be used from the @mastra/memory class. So I can put this environment variable there.
| metadata: Record<string, unknown>; | ||
| }; | ||
|
|
||
| export class InMemoryVector extends MastraVector { |
There was a problem hiding this comment.
Why can we not use the same idea for this as we do for regular storage and use libsql?
There was a problem hiding this comment.
Left more context in the package.json. tl;dr is the goal is to not depend on @libsql/client unless it's necessary
packages/core/package.json
Outdated
| @@ -130,6 +129,14 @@ | |||
| "xstate": "^5.19.2", | |||
| "zod": "^3.24.2" | |||
| }, | |||
| "peerDependencies": { | |||
| "@libsql/client": "^0.14.0" | |||
There was a problem hiding this comment.
I don't know why we need to do this. Adding these lines doesn't fail the install step when it cannot be installed, but it will still install it on almost every platform. I'm not sure why convex is different.
There was a problem hiding this comment.
@libsql/client is a big dependency (30MB) that bloats the bundle size, which is particularly bad for serverless environments (it ends up exceeding the Convex bundle size limit). This package is really big and doesn't need to be installed unless you're using the Storage or Vector implementation that uses libsql (by comparison, pg is .5MB). No storage provider currently uses @libsql/client except the ones that interact with file-based databases.
Instead of making it optional overall, this PR could instead focus on:
- Making
@libsql/clientnot the default when you're running in an environment where the env vars aren't set (not mastra dev, e.g. in a Convex serverless function). - Ensure there's a way to deploy Mastra to Convex-like environments where it doesn't include
@libsql/client. Two options for that:
a. Server install: having it configurable to not install it server-side, as it pulls it in by default currently.
b. Local bundling: Figuring out how to get@libsql/clientto analyze correctly on my mac and let it tree-shake it out if it isn't used at import time.
(1) is solved with in-memory defaults outside of mastra dev
(2) is an open question, if we don't do the import obfuscation
This reverts commit 5d9e803.
|
The repo has the in-memory implementations (pending tests if it's the right direction), but still lacks the ability to build locally for a cloud deploy (due to The hacks by which I've gotten it to work:
"overrides": {
"@libsql/client": "file:libsql-client-0.14.0-noop.tgz"
},where I made a "noop" implementation of Another route here would be to make browser-compatible targets for all entrypoints. That's a big undertaking, as it'd require replacing all dependencies relying on a node.js environment with browser-compatible equivalents, like subbing out |
|
The test failures all seemed to be related to setting environment variables, which I'm assuming I'd need to set in my own GitHub environment. What's the best way to run them in your github org? |
Aiming to make
@libsql/clientan opt-in dependency that isn't bundled by default, without losing the "default" storage or vector stores which are useful for getting started.This involves two efforts:
The strategy for backwards compatibility is to use the dev env vars as a signal as to whether to use the files with libsql or do the in-memory version. Serverless deploys won't have the environment variables present (unless they set them intentionally).
Backwards compatibility story:
mastra devwill still default into an on-disk experience with a new env variable to mirror the storage urlFixes #2968