Skip to content

Add alternative ways ('TE:' and 'EMB:') to load embeddings in prompt …#14745

Open
silveroxides wants to merge 3 commits into
Comfy-Org:masterfrom
silveroxides:feature/alt-embedding-id
Open

Add alternative ways ('TE:' and 'EMB:') to load embeddings in prompt …#14745
silveroxides wants to merge 3 commits into
Comfy-Org:masterfrom
silveroxides:feature/alt-embedding-id

Conversation

@silveroxides

Copy link
Copy Markdown
Contributor

…since using 'embedding:' is kind of lengthy and cumbersome to use.

This makes "embedding:", "EMB:" and "TE:" all valid identifiers when wanting to load embeddings in prompt.

Here is example if #14744 has not been merged

example_14744_notmerged.json

Here is example if #14744 has been merged (Which might still require me to make minor conflict resolution though)

example_14744_merged.json

Here is the embedding used in both workflows

qwen3vl_4b_duckonbear.safetensors

…since using 'embedding:' is kind of lengthy and cumbersome to use
@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 274b940d-3f2e-4177-b749-5067b6c416fb

📥 Commits

Reviewing files that changed from the base of the PR and between 7d78232 and 7605b99.

📒 Files selected for processing (1)
  • comfy/sd1_clip.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • comfy/sd1_clip.py

📝 Walkthrough

Walkthrough

comfy/sd1_clip.py now supports multiple embedding prefix identifiers instead of a single hardcoded prefix. SDTokenizer adds an embedding_identifiers property that includes the existing identifier plus EMB: and TE:. tokenize_with_weights now splits text with a regex built from those identifiers, identifies which prefix matched each fragment, and derives embedding_name from the matched prefix.

Changes

Area Change
comfy/sd1_clip.py Added embedding_identifiers; updated embedding-prefixed token splitting and name extraction

Sequence Diagram(s)

Not applicable.

Estimated code review effort: 2

Related PRs: None identified.

Suggested labels: tokenizer, enhancement

Suggested reviewers: comfyanonymous

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: adding alternate embedding prefixes for prompt loading.
Description check ✅ Passed The description directly explains the same embedding-prefix change and includes relevant examples.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@comfy/sd1_clip.py`:
- Line 538: The new embedding prefixes in sd1_clip.py are too generic and can be
misread from normal prompt text. Update the prefix handling in the logic around
embedding_identifiers and the parser that checks for preceding whitespace so
that "EMB:" and "TE:" only match with a stronger boundary or other
disambiguation, and do not turn ordinary prompt fragments into embedding
lookups. Keep the existing "embedding:" behavior intact while making the
abbreviated identifiers collision-safe in the embedding resolution path.
- Around line 537-538: The embedding prefix override hook is broken because
tokenize_with_weights now relies on embedding_identifiers instead of the single
source-of-truth embedding_identifier. Update sd1_clip.py so
tokenize_with_weights in the SD1 clip tokenizer derives its prefix checks from
self.embedding_identifier first, or builds the identifier list from that
singular attribute, preserving subclass/custom tokenizer overrides while keeping
the existing EMB:/TE: aliases.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 70e3fba5-8a98-44b6-8d29-d07774c713f9

📥 Commits

Reviewing files that changed from the base of the PR and between 77917ed and 2256608.

📒 Files selected for processing (1)
  • comfy/sd1_clip.py

Comment thread comfy/sd1_clip.py Outdated
Comment thread comfy/sd1_clip.py Outdated
… self.embedding_identifier as the singular source of truth. Added dynamic embedding_identifiers property.
@alexisrolland

Copy link
Copy Markdown
Member

I am not super fan of this:

  • Enforcing one way of doing things brings some simplicity.
  • embedding: is longer to type but it has the benefit of making things explicit.
  • It also matches the folder name which is a nice hint.
  • If we were to support this, I think a better solution would be to modify the identifier directly self.embedding_identifier = "embedding:"

I would suggest to close it.

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