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

feature extraction, sentence similarity and text classification now use pipeline url #197

Closed
wants to merge 0 commits into from

Conversation

lisandropuzzolo
Copy link
Contributor

Some models allows them to be use on more than one (default task).
In order to let the huggingface.js use more than the default model task, I've changed featureExtraction, sentenceSimilarity and textClassification to change the model to the pipeline url when model is not a url.

Copy link
Member

@coyotte508 coyotte508 left a comment

Choose a reason for hiding this comment

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

Thanks, that's a neat feature!

I would add a pipeline optional string param here:
image

And do the logic of changing the url in makeRequestOptions instead.

In textClassification etc, it would be a one-line-change:

const res = (await request<TextClassificationOutput[]>(args, {
  ...options, 
  pipeline: "text-classification" // HERE!
}))?.[0];

packages/inference/src/lib/makeRequestOptions.ts Outdated Show resolved Hide resolved
@lisandropuzzolo
Copy link
Contributor Author

Done changes sugested by coyotte508

Copy link
Member

@coyotte508 coyotte508 left a comment

Choose a reason for hiding this comment

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

A few comments left, after they're handled I'll update tapes.json and merge !

Comment on lines 7 to 17
function isModelTypeURL(model: string): boolean {
return /^http(s?):/.test(model) || model.startsWith("/");
}

function getPipelineURL(model: string, pipeline: string): string {
return `${HF_INFERENCE_API_PIPELINE_BASE_URL}${pipeline}/${model}`;
}

function getDefaultModelTaskURL(model: string) {
return `${HF_INFERENCE_API_MODEL_BASE_URL}${model}`;
}
Copy link
Member

Choose a reason for hiding this comment

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

Those are one-line local functions used once, can you hardcode them directly in makeRequestOptions?

@@ -2,6 +2,8 @@ import { InferenceOutputError } from "../../lib/InferenceOutputError";
import type { BaseArgs, Options } from "../../types";
import { request } from "../custom/request";

const HF_INFERENCE_API_PIPELINE_FEATURE_EXTRACTION = "feature-extraction";
Copy link
Member

Choose a reason for hiding this comment

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

The constant is only used once / not worth exporting, can you hardcode it below and remove the declaration here?

(maybe later we'll type pipeline as "feature-extraction" | ... | string to offer some kind of completion)

Same remark for the two other tasks

Copy link
Member

@julien-c julien-c left a comment

Choose a reason for hiding this comment

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

would be interested in @Narsil (and @OlivierDehaene) feedback and maybe @Wauplin too on this

is this a feature of the Inference API we want to expose?

@lisandropuzzolo
Copy link
Contributor Author

would be interested in @Narsil (and @OlivierDehaene) feedback and maybe @Wauplin too on this

is this a feature of the Inference API we want to expose?

it's a very needed inference api feature, because many models can be used for more than one task

@Wauplin
Copy link
Contributor

Wauplin commented May 19, 2023

Makes me think about this recent convo (huggingface/huggingface_hub#1474 (comment)). In huggingface_hub we'll use the /task endpoints to allow this.

As stated here, we might not use it in the end.

@coyotte508
Copy link
Member

Sorry @lisandropuzzolo wrong git manipulation 😬

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.

4 participants