-
Notifications
You must be signed in to change notification settings - Fork 2
feat(attachments): create context-aware helper for attachments #44
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
Changes from 5 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
import axios, { AxiosError } from 'axios'; | ||
import FormData from 'form-data'; | ||
import { createReadStream } from 'fs'; | ||
import { ReadStream } from 'fs'; | ||
import { v4 as uuidv4 } from 'uuid'; | ||
|
||
import { LiteralClient } from '.'; | ||
|
@@ -21,6 +22,7 @@ import { | |
PersistedGeneration | ||
} from './generation'; | ||
import { | ||
Attachment, | ||
CleanThreadFields, | ||
Dataset, | ||
DatasetExperiment, | ||
|
@@ -327,6 +329,28 @@ function addGenerationsToDatasetQueryBuilder(generationIds: string[]) { | |
`; | ||
} | ||
|
||
type UploadFileBaseParams = { | ||
id?: Maybe<string>; | ||
threadId?: string; | ||
mime?: Maybe<string>; | ||
}; | ||
type UploadFileParamsWithPath = UploadFileBaseParams & { | ||
path: string; | ||
}; | ||
type UploadFileParamsWithContent = UploadFileBaseParams & { | ||
content: | ||
| ReadableStream<any> | ||
| ReadStream | ||
| Buffer | ||
| File | ||
| Blob | ||
| ArrayBuffer; | ||
}; | ||
type CreateAttachmentParams = { | ||
name?: string; | ||
metadata?: Maybe<Record<string, any>>; | ||
}; | ||
|
||
export class API { | ||
/** @ignore */ | ||
private client: LiteralClient; | ||
|
@@ -596,19 +620,25 @@ export class API { | |
* @returns An object containing the `objectKey` of the uploaded file and the signed `url`, or `null` values if the upload fails. | ||
* @throws {Error} Throws an error if neither `content` nor `path` is provided, or if the server response is invalid. | ||
*/ | ||
|
||
async uploadFile(params: UploadFileParamsWithContent): Promise<{ | ||
objectKey: Maybe<string>; | ||
url: Maybe<string>; | ||
}>; | ||
async uploadFile(params: UploadFileParamsWithPath): Promise<{ | ||
objectKey: Maybe<string>; | ||
url: Maybe<string>; | ||
}>; | ||
async uploadFile({ | ||
Dam-Buty marked this conversation as resolved.
Show resolved
Hide resolved
|
||
content, | ||
path, | ||
id, | ||
threadId, | ||
mime | ||
}: { | ||
content?: Maybe<any>; | ||
path?: Maybe<string>; | ||
id?: Maybe<string>; | ||
threadId: string; | ||
mime?: Maybe<string>; | ||
}) { | ||
}: UploadFileParamsWithContent & UploadFileParamsWithPath): Promise<{ | ||
objectKey: Maybe<string>; | ||
url: Maybe<string>; | ||
}> { | ||
if (!content && !path) { | ||
throw new Error('Either content or path must be provided'); | ||
} | ||
|
@@ -678,6 +708,63 @@ export class API { | |
} | ||
} | ||
|
||
async createAttachment( | ||
params: UploadFileParamsWithContent & CreateAttachmentParams | ||
): Promise<Attachment>; | ||
async createAttachment( | ||
params: UploadFileParamsWithPath & CreateAttachmentParams | ||
): Promise<Attachment>; | ||
async createAttachment( | ||
params: UploadFileParamsWithContent & | ||
UploadFileParamsWithPath & | ||
CreateAttachmentParams | ||
): Promise<Attachment> { | ||
if (params.content instanceof Blob) { | ||
params.content = Buffer.from(await params.content.arrayBuffer()); | ||
} | ||
if (params.content instanceof ArrayBuffer) { | ||
params.content = Buffer.from(params.content); | ||
} | ||
|
||
let threadFromStore: Thread | null = null; | ||
try { | ||
threadFromStore = this.client.getCurrentThread(); | ||
} catch (error) { | ||
// Ignore error thrown if getCurrentThread is called outside of a context | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I debug, I expect caught errors to appear somewhere in the logs, it helps me navigate through the code and quickly identify the location of a bug. I think it's only me in the team though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case, there is genuinely nothing to log. You may or may not be inside of a context, both of which are valid states. But you're right, that's a code smell so i added I couldn't make them private (as they are consumed from outside of the LiteralClient class) but they are undocumented. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, it's like using try/catch as an if condition. |
||
} | ||
|
||
let stepFromStore: Step | null = null; | ||
try { | ||
stepFromStore = this.client.getCurrentStep(); | ||
} catch (error) { | ||
// Ignore error thrown if getCurrentStep is called outside of a context | ||
} | ||
|
||
if (threadFromStore) { | ||
params.threadId = threadFromStore.id; | ||
} | ||
|
||
const { objectKey, url } = await this.uploadFile(params); | ||
|
||
const attachment = new Attachment({ | ||
name: params.name, | ||
objectKey, | ||
mime: params.mime, | ||
metadata: params.metadata, | ||
url | ||
}); | ||
|
||
if (stepFromStore) { | ||
if (!stepFromStore.attachments) { | ||
stepFromStore.attachments = []; | ||
} | ||
|
||
stepFromStore.attachments.push(attachment); | ||
} | ||
|
||
return attachment; | ||
} | ||
|
||
// Generation | ||
/** | ||
* Retrieves a paginated list of Generations based on the provided filters and sorting order. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,96 @@ | ||
import 'dotenv/config'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you so much for splitting tests in their own file! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you're welcome, i'm unable to focus on a 700 line test file anyway 😅 |
||
import { createReadStream, readFileSync } from 'fs'; | ||
|
||
import { Attachment, LiteralClient, Maybe } from '../src'; | ||
|
||
const url = process.env.LITERAL_API_URL; | ||
const apiKey = process.env.LITERAL_API_KEY; | ||
if (!url || !apiKey) { | ||
throw new Error('Missing environment variables'); | ||
} | ||
const client = new LiteralClient(apiKey, url); | ||
|
||
const filePath = './tests/chainlit-logo.png'; | ||
const mime = 'image/png'; | ||
|
||
function removeVariableParts(url: string) { | ||
return url.split('X-Amz-Date')[0].split('X-Goog-Date')[0]; | ||
} | ||
|
||
describe('Attachments', () => { | ||
describe('Uploading a file', () => { | ||
const stream = createReadStream(filePath); | ||
const buffer = readFileSync(filePath); | ||
const arrayBuffer = buffer.buffer; | ||
const blob = new Blob([buffer]); | ||
// We wrap the blob in a blob and simulate the structure of a File | ||
const file = new Blob([blob], { type: 'image/jpeg' }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. png maybe? |
||
|
||
it.each([ | ||
{ type: 'Stream', content: stream! }, | ||
{ type: 'Buffer', content: buffer! }, | ||
{ type: 'ArrayBuffer', content: arrayBuffer! }, | ||
{ type: 'Blob', content: blob! }, | ||
{ type: 'File', content: file! } | ||
])('handles $type objects', async function ({ type, content }) { | ||
const attachment = await client.api.createAttachment({ | ||
content, | ||
mime, | ||
name: `Attachment ${type}`, | ||
metadata: { type } | ||
}); | ||
|
||
const step = await client | ||
.run({ | ||
name: `Test ${type}`, | ||
attachments: [attachment] | ||
}) | ||
.send(); | ||
|
||
await new Promise((resolve) => setTimeout(resolve, 1000)); | ||
|
||
const fetchedStep = await client.api.getStep(step.id!); | ||
|
||
const urlWithoutVariables = removeVariableParts(attachment.url!); | ||
const fetchedUrlWithoutVariables = removeVariableParts( | ||
fetchedStep?.attachments![0].url as string | ||
); | ||
|
||
expect(fetchedStep?.attachments?.length).toBe(1); | ||
expect(fetchedStep?.attachments![0].objectKey).toEqual( | ||
attachment.objectKey | ||
); | ||
expect(fetchedStep?.attachments![0].name).toEqual(attachment.name); | ||
expect(fetchedStep?.attachments![0].metadata).toEqual( | ||
attachment.metadata | ||
); | ||
expect(urlWithoutVariables).toEqual(fetchedUrlWithoutVariables); | ||
}); | ||
}); | ||
|
||
describe('Handling context', () => { | ||
it('attaches the attachment to the step in the context', async () => { | ||
const stream = createReadStream(filePath); | ||
|
||
let stepId: Maybe<string>; | ||
let attachment: Maybe<Attachment>; | ||
|
||
await client.run({ name: 'Attachment test ' }).wrap(async () => { | ||
stepId = client.getCurrentStep().id!; | ||
attachment = await client.api.createAttachment({ | ||
content: stream!, | ||
mime, | ||
name: 'Attachment', | ||
metadata: { type: 'Stream' } | ||
}); | ||
}); | ||
|
||
await new Promise((resolve) => setTimeout(resolve, 1000)); | ||
|
||
const fetchedStep = await client.api.getStep(stepId!); | ||
|
||
expect(fetchedStep?.attachments?.length).toBe(1); | ||
expect(fetchedStep?.attachments![0].id).toEqual(attachment!.id); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's still a png, what was the trick to reduce size? # of RGB channels? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes ! i used one of those online squashers, and reduced the color depth (to <10 colors iirc) |
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 didn't know we could declare prototypes for doc/autocompletion purposes.
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.
Just realized you mentioned "Fix typing to enforce either content or path at compile time" -> very clear.
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.
yes overloading function types is a great way to handle this kind of either/or parameters. It's a bit verbose but the end result is satisfying from the POV of the dev using the function.