-
Notifications
You must be signed in to change notification settings - Fork 12
Feat: Use thread unique workspace, add knowledge ingest #201
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
Feat: Use thread unique workspace, add knowledge ingest #201
Conversation
a2e7c4a to
947167e
Compare
|
|
||
| const execPromise = promisify(exec); | ||
|
|
||
| export async function ingest( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where we run knowledge ingest command on server side to ingest file on fly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great 👍
I left a few comments, but besides that I'd like to check to make sure the new binaries don't interfere with notarizing before this goes in. I'll circle back with the results shortly.
Edit:
Just checked and it passes code signing and notarization for mac 💯
targetPlatform: mac
• electron-builder version=24.13.3 os=23.6.0
• description is missed in the package.json appPackageFile=/Users/nick/projects/gptscript-ai/desktop/package.json
• writing effective config file=electron-dist/builder-effective-config.yaml
• packaging platform=darwin arch=arm64 electron=31.3.1 appOutDir=electron-dist/mac-arm64
• signing file=electron-dist/mac-arm64/Acorn.app platform=darwin type=distribution identity=A96259356AEA4477EE16007EBEF6012A993E089E provisioningProfile=none
• notarization successful
• building target=DMG arch=arm64 file=electron-dist/Acorn-0.10.0-rc3-arm64.dmg
• Detected arm64 process, HFS+ is unavailable. Creating dmg with APFS - supports Mac OSX 10.12+
Signed-off-by: Daishan Peng <daishan@acorn.io>
Signed-off-by: Daishan Peng <daishan@acorn.io>
947167e to
b9974c0
Compare
| await fs.writeFile(path.join(threadPath, STATE_FILE), ''); | ||
|
|
||
| if (firstMessage) { | ||
| const generatedThreadName = await generateThreadName(firstMessage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand it, it was a requirement to have the LLM name the thread based on the first message. Has that requirement changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I changed it because the thread will always be created at first and it is kind of weird that the name will be changed later after user type the message. But yeah I will confirm with craig.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will change this, thanks for pointting out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
| prompt: true, | ||
| confirm: true, | ||
| env: [ | ||
| ...Object.entries(process.env).map(([key, value]) => `${key}=${value}`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be necessary. Adding env vars here should append to the existing environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the code and it doesn't seem so. https://github.com/gptscript-ai/node-gptscript/blob/main/src/gptscript.ts#L88
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It happens elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but since I'm new to the code base, I'd wait for @njhale's and @thedadams' final review :)
Signed-off-by: Daishan Peng <daishan@acorn.io>
This pr adds the following features/changes/improvements:
GPTSCRIPT_THREAD_IDto gptscript run so that it can be used by knowledge tool to know which dataset it needs to query from.knowledgesubdir inside its own workspace.