Skip to content

update snippets for image-text-to-text models for transformers models #1434

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 12 commits into from
May 9, 2025

Conversation

Vaibhavs10
Copy link
Member

No description provided.

@Vaibhavs10 Vaibhavs10 requested a review from mishig25 May 6, 2025 15:07
@Vaibhavs10 Vaibhavs10 requested a review from pcuenca May 7, 2025 09:19
if (model.tags.includes("conversational") && model.config?.tokenizer_config?.chat_template) {
pipelineSnippet.push("pipe(messages)");
}
pipelineSnippet.push("pipe(messages)");
Copy link
Member

Choose a reason for hiding this comment

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

hmm, I think this line is still out of the conversational path. How about something like this? (forgive mistakes, typing in this text box)

		const pipelineSnippet = ["# Use a pipeline as a high-level helper", "from transformers import pipeline", ""];
		pipelineSnippet.push(`pipe = pipeline("${model.pipeline_tag}", model="${model.id}"` + remote_code_snippet + ")");

		if (model.config?.tokenizer_config?.chat_template && model.tags.includes("conversational")) {
			if (model.tags.includes("image-text-to-text")) {
				pipelineSnippet.push(
					"messages = [",
					[
						"    {",
						'        "role": "user",',
						'        "content": [',
						'            {"type": "image", "url": "https://huggingface.co/datasets/huggingface/documentation-images/resolve/main/p-blog/candy.JPG"},',
						'            {"type": "text", "text": "What animal is on the candy?"}',
						"        ]",
						"    },",
					].join("\n"),
					"]"
				);
			} else {
				pipelineSnippet.push("messages = [", '    {"role": "user", "content": "Who are you?"},', "]");
			}
			pipelineSnippet.push("pipe(messages)");
		}

		return [pipelineSnippet.join("\n"), autoSnippet];

Copy link
Member Author

Choose a reason for hiding this comment

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

It isn't no? https://gist.github.com/Vaibhavs10/b525c1ae4982c04fe0202c2f78ac2f03

the pipeline instantiation and the pipe(messages) is the same for both

(my JS sucks so could very well be a skill issue)

Copy link
Member

@pcuenca pcuenca May 7, 2025

Choose a reason for hiding this comment

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

lol, I'm probably blind by now, but this is the way I'm seeing it:

Screenshot 2025-05-07 at 15 40 37

i.e., if there is no chat_template, there's no messages.

Also, if conversational is not present (a mistake, probably) we wouldn't have messages either, that's why I suggested to require it in the outer if too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggested another variation based on your previous comments here: https://gist.github.com/Vaibhavs10/b525c1ae4982c04fe0202c2f78ac2f03?permalink_comment_id=5569573#gistcomment-5569573

Changes are:

  • the pipe = pipeline(... line should be added at the beginning => no matter the pipeline_tag we want to instantiate the pipeline
  • the pipe(messages) line should be added only if conversational mode as @pcuenca mentioned above
  • I would not check model.config?.tokenizer_config?.chat_template manually but rely only on the model.tags.includes("conversational") condition. This makes the logic simpler and it should be equivalent since moon-landing is automatically adding conversational when chat template is detected (see private link) (it's even better because moon-landing checks for both tokenizer_config and processor_config and is maintained to be future-proof)

Copy link
Member

Choose a reason for hiding this comment

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

yes^

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm stupid for not looking at that code block - sorry for being so dense!

Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

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

Looks good!

@Vaibhavs10 Vaibhavs10 merged commit d9dcbf2 into main May 9, 2025
4 of 5 checks passed
@Vaibhavs10 Vaibhavs10 deleted the vb/snippets branch May 9, 2025 12:52
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