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

Refacto Inference snippets tests (autogeneration + 1 snippet == 1 file) #1046

Merged
merged 8 commits into from
Nov 25, 2024

Conversation

Wauplin
Copy link
Contributor

@Wauplin Wauplin commented Nov 22, 2024

This PR is a refacto of the inference snippet tests. The main goal is to reduce friction when updating the inference snippets logic. In particular:

  • each generated snippet is saved into a single test file with the correct file extension => should greatly improve reviews. Currently snippets are saved as hardcoded strings in JS files, making them hard to review (no syntax highlighting, weird indentation).
  • snippets can be updated with a script, reducing the need to manually edit tests (which is quite time-consuming)

This PR is quite large given the auto-generated files. The main parts to review are:

  • generate-snippets-fixtures.ts => the script that generates the snippets and test them.
  • package.json, pnpm lock files, etc. => I'm not entirely sure of what I did there (especially to make tasks-gen depend on tasks)
  • python.spec.ts / js.specs.ts and curl.specs.ts have been removed
  • everything in packages/tasks-gen/snippets-fixtures/ => the test cases. I've only committed the ones that where previously tested.

Thanks to this PR, I fixed a typo in the JS snippets (a missing comma) and curl snippets (consistency between " and ').

cc @mishig25 with whom I quickly discussed this

@Wauplin Wauplin marked this pull request as draft November 22, 2024 16:25
@Wauplin Wauplin marked this pull request as ready for review November 22, 2024 16:29
import type { InferenceSnippet } from "@huggingface/tasks";
import { snippets } from "@huggingface/tasks";

type LANGUAGE = "sh" | "js" | "py";
Copy link
Collaborator

Choose a reason for hiding this comment

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

for the info, this is how the same thing is declared in moon. In subseq PR, we can probably declare InferenceSnippetLanguage in tasksand use it in tasks-gen & moon

export const inferenceSnippetLanguages = ["python", "js", "curl"] as const;

export type InferenceSnippetLanguage = (typeof inferenceSnippetLanguages)[number];

Copy link
Collaborator

@mishig25 mishig25 left a comment

Choose a reason for hiding this comment

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

lgtm !
thanks for this refactor

@Wauplin
Copy link
Contributor Author

Wauplin commented Nov 25, 2024

thanks for the review :)

@Wauplin Wauplin merged commit c63f9ae into main Nov 25, 2024
5 checks passed
@Wauplin Wauplin deleted the inference-snippets-standalone-tests branch November 25, 2024 11:35
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.

2 participants