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

Add support for files API endpoints #184

Merged
merged 5 commits into from
May 22, 2024
Merged

Add support for files API endpoints #184

merged 5 commits into from
May 22, 2024

Conversation

mattt
Copy link
Member

@mattt mattt commented Jan 9, 2024

In #198, we added support for automatically encoding binary prediction inputs as base64-encoded data URIs.

This PR adds support for the new (write-only) files API. By default, each binary input is uploaded and replaced with a URL to the created file. Before the inputs are sent to the model to be run, Replicate's API rewrites the URL to make the file downloadable.

From the perspective of the caller, the out-of-the-box experience is the same: You pass a file handle or blog as an input, and that's it — everything just works. And with the files API, things work much better for larger files, whose data URI encoding can cause write timeouts and other problems.

What I'm less clear about is how to customize this behavior. My current solution is two-fold:

  • Add a prepareInputs property to the client
  • Export the transformFileInputsToBase64EncodedDataURIs and transformFileInputsToReplicateFileURLs helper functions

By default, prepareInputs tries to upload files and falls back to data URI encoding (for example if the files API is unavailable for some reason).

this.prepareInputs =
  options.prepareInputs ||
  (async (inputs) => {
    try {
      return await transformFileInputsToReplicateFileURLs(this, inputs);
    } catch (error) {
      return await transformFileInputsToBase64EncodedDataURIs(inputs);
    }
  });

Users can override this to customize the behavior however they like. For example, they could only base64-encode inputs, conditionalize based on file size / content, or throw an error.

I'm pretty sure we need it to be this flexible. But is it too clunky? Would love to hear your thoughts, @aron @zeke.

@mattt mattt requested a review from zeke January 9, 2024 00:46
index.d.ts Outdated Show resolved Hide resolved
@mattt mattt force-pushed the mattt/files branch 2 times, most recently from fe780bb to e5f221f Compare January 9, 2024 01:02
@mattt mattt marked this pull request as ready for review February 17, 2024 14:56
@mattt mattt requested review from a team, aron and zeke February 21, 2024 11:40
Copy link
Member

@zeke zeke left a comment

Choose a reason for hiding this comment

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

Nice! I may not be the right power-user customer to ask about that prepareInputs API but it looks sensible to me. 👍🏼

Needs docs.

Just came across @aron's #198 which already updated the README to use this glamorous snippet:

const input = {
  image: await fs.readFile("path/to/image.png"),
};

Copy link
Contributor

@aron aron left a comment

Choose a reason for hiding this comment

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

Exciting!!!

Some late night thoughts, I think prepareInputs needs a second pass. And I do think we should consider the surface area of the files api but otherwise I am so excited to get this in.

index.d.ts Outdated Show resolved Hide resolved
lib/files.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
lib/files.js Show resolved Hide resolved
lib/files.js Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.test.ts Show resolved Hide resolved
@aron
Copy link
Contributor

aron commented Mar 5, 2024

@mattt I've incorporated the feedback, updated the code to use a FileUploadStrategy option which can consist of "default" | "upload" | "data-uri".

Types have been updated and the tests now verify the FormData body, this was a bit fiddly because nock doesn't support it natively so I had to spy on fetch() too.

@aron
Copy link
Contributor

aron commented Mar 11, 2024

@mattt I've removed the replicate.files API for the moment from the client. I think this is a good first step to improve our file input support.

mattt and others added 5 commits May 2, 2024 16:14
This allows the user to determine if file uploads, falling back to
base64 should be used or just sticking to one approach.

The tests have been updated to validate the file upload payload and
to ensure the url is correctly passed to the prediction create method.
@zeke
Copy link
Member

zeke commented May 2, 2024

Rebased and fixed merge conflicts. Anything blocking this from shipping?

@zeke
Copy link
Member

zeke commented May 22, 2024

@mattt bump

@mattt mattt merged commit cc3d281 into main May 22, 2024
18 checks passed
@mattt mattt deleted the mattt/files branch May 22, 2024 21:04
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.

None yet

3 participants