Skip to content

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

Merged
merged 6 commits into from
Jul 25, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 94 additions & 7 deletions src/api.ts
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 '.';
Expand All @@ -21,6 +22,7 @@ import {
PersistedGeneration
} from './generation';
import {
Attachment,
CleanThreadFields,
Dataset,
DatasetExperiment,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<{
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

objectKey: Maybe<string>;
url: Maybe<string>;
}>;
async uploadFile(params: UploadFileParamsWithPath): Promise<{
objectKey: Maybe<string>;
url: Maybe<string>;
}>;
async uploadFile({
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');
}
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 _currentThread and _currentStep methods which do not throw outside of context. It is much cleaner.

I couldn't make them private (as they are consumed from outside of the LiteralClient class) but they are undocumented.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.
Expand Down
25 changes: 10 additions & 15 deletions src/openai.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,21 +42,16 @@ class OpenAIAssistantSyncer {
);
const mime = 'image/png';

const { objectKey } = await this.client.api.uploadFile({
threadId: litThreadId,
id: attachmentId,
content: file.body,
mime
});

const attachment = new Attachment({
name: content.image_file.file_id,
id: attachmentId,
objectKey,
mime
});

attachments.push(attachment);
if (file.body) {
const attachment = await this.client.api.createAttachment({
threadId: litThreadId,
id: attachmentId,
content: file.body,
mime
});

attachments.push(attachment);
}
} else if (content.type === 'text') {
output.content += content.text.value;
}
Expand Down
45 changes: 1 addition & 44 deletions tests/api.test.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,7 @@
import 'dotenv/config';
import { createReadStream } from 'fs';
import { v4 as uuidv4 } from 'uuid';

import {
Attachment,
ChatGeneration,
Dataset,
LiteralClient,
Score
} from '../src';
import { ChatGeneration, Dataset, LiteralClient, Score } from '../src';

describe('End to end tests for the SDK', function () {
let client: LiteralClient;
Expand Down Expand Up @@ -336,42 +329,6 @@ describe('End to end tests for the SDK', function () {
expect(scores[1].scorer).toBe('openai:gpt-3.5-turbo');
});

it('should test attachment', async function () {
const thread = await client.thread({ id: uuidv4() });
// Upload an attachment
const fileStream = createReadStream('./tests/chainlit-logo.png');
const mime = 'image/png';

const { objectKey } = await client.api.uploadFile({
threadId: thread.id,
content: fileStream,
mime
});

const attachment = new Attachment({
name: 'test',
objectKey,
mime
});

const step = await thread
.step({
name: 'test',
type: 'run',
attachments: [attachment]
})
.send();

await new Promise((resolve) => setTimeout(resolve, 1000));

const fetchedStep = await client.api.getStep(step.id!);
expect(fetchedStep?.attachments?.length).toBe(1);
expect(fetchedStep?.attachments![0].objectKey).toBe(objectKey);
expect(fetchedStep?.attachments![0].url).toBeDefined();

await client.api.deleteThread(thread.id);
});

it('should get project id', async () => {
const projectId = await client.api.getProjectId();
expect(projectId).toEqual(expect.any(String));
Expand Down
96 changes: 96 additions & 0 deletions tests/attachments.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
import 'dotenv/config';
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you so much for splitting tests in their own file!

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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' });
Copy link
Contributor

Choose a reason for hiding this comment

The 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);
});
});
});
Binary file modified tests/chainlit-logo.png
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading