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

Move inference snippets logic to inference package #1247

Merged
merged 8 commits into from
Mar 4, 2025

Conversation

Wauplin
Copy link
Contributor

@Wauplin Wauplin commented Mar 3, 2025

Original slack message from @Wauplin (private link):

I have an annoying problem I don't know how to handle in @hugingface.js.
I'm working on updating the inference snippets under @tasks . Now that we have a makeUrl helper for each provider (see cohere example), I want to use it in the CURL snippets to display to correct URL (for now it's hardcoded here which is incorrect for some providers). To do that, I need to make @tasks depend on @inference . But @inference is already depending on @tasks especially to type inference inputs/outputs. Is this something npm/pnpm is able to settle (in Python it's not ^^).

The other solution I'm thinking about is to start having a @snippets package depending on both @tasks and @inference but I'd like to avoid that as much as possible 🙈 Any idea?

=> after some discussions we went for "let's move the snippets code to inference" which this PR does. @julien-c @coyotte508

⚠️ This is a breaking change for @huggingface.js/tasks. Will require to make a major release?


In practice I had to move only parts of the ./snippets folder:

  • the type definitions + utils remains in @tasks since it is used to generate some local app snippets
  • only the js/python/curl -specific parts are moved to @inference => i.e. only the logic to generate the snippets, not the helpers

@Wauplin
Copy link
Contributor Author

Wauplin commented Mar 3, 2025

EDIT: I just pushed d5fb4d6 to fix https://github.com/huggingface/huggingface.js/actions/runs/13636239608/job/38115630313?pr=1247:
=> Reverted it, the issue still exists 😕

Error: node_modules/.pnpm/@[email protected]/node_modules/@huggingface/tasks/src/model-libraries-snippets.ts(354,53): error TS2550: Property 'replaceAll' does not exist on type 'string'. Do you need to change your target library? Try changing the 'lib' compiler option to 'es2021' or later.
Error: node_modules/.pnpm/@[email protected]/node_modules/@huggingface/tasks/src/snippets/common.ts(13,29): error TS2550: Property 'replaceAll' does not exist on type 'string'. Do you need to change your target library? Try changing the 'lib' compiler option to 'es2021' or later.
Error: node_modules/.pnpm/@[email protected]/node_modules/@huggingface/tasks/src/snippets/inputs.ts(161,37): error TS1501: This regular expression flag is only available when targeting 'es2018' or later.

I chose to set ES2022 as this is what's already defined for @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.

Works for me, though I still think we should merge those two packages...

This is a breaking change for @huggingface.js/tasks. Will require to make a major release?

I highly doubt anyone outside of HF was relying on the snippets being inside of tasks

For that matter, I doubt anyone outside of HF is importing tasks (it's not really a library or natural building block imo)

@Wauplin Wauplin changed the title [draft] move inference snippets logic to inference package Move inference snippets logic to inference package Mar 4, 2025
@Wauplin
Copy link
Contributor Author

Wauplin commented Mar 4, 2025

I'll merge as-is for now and I'll consider merging everything together if I ever need to move parts again

@Wauplin Wauplin merged commit d05b840 into main Mar 4, 2025
5 checks passed
@Wauplin Wauplin deleted the move-inference-snippets-logic-to-inference-package branch March 4, 2025 13:31
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.

3 participants