fix(janus): Handle None values in image generation mode#44793
Closed
BillionClaw wants to merge 1 commit intohuggingface:mainfrom
Closed
fix(janus): Handle None values in image generation mode#44793BillionClaw wants to merge 1 commit intohuggingface:mainfrom
BillionClaw wants to merge 1 commit intohuggingface:mainfrom
Conversation
Fix several issues in JanusForConditionalGeneration.generate() when generation_config has None values: - Handle num_return_sequences=None by defaulting to 1 - Add safety checks for generation_kwargs and boi_token_id - Handle pad_token_id=None by falling back to config value - Fix max_cache_len calculation when max_length is None Fixes huggingface#44792
Contributor
|
[For maintainers] Suggested jobs to run (before merge) run-slow: janus |
Author
|
Hi! Just checking in on this PR. Is there anything needed from my side to help move it forward? |
Member
zucchini-nlp
left a comment
zucchini-nlp
left a comment
There was a problem hiding this comment.
omg, i didn't post comments and they were pending 🤦🏻
| input_ids=input_ids, | ||
| attention_mask=attention_mask, | ||
| expand_size=generation_config.num_return_sequences, | ||
| expand_size=generation_config.num_return_sequences or 1, |
Member
There was a problem hiding this comment.
we just need to call _prepare_generation_config in the very beginning, it is missing
| attention_mask = attention_mask.repeat(2, 1) | ||
| model_kwargs["attention_mask"] = attention_mask | ||
|
|
||
| # Ensure generation_kwargs exists with boi_token_id |
Member
There was a problem hiding this comment.
prob this and rest will be resolved after calling _prepare_generation_config
|
Friendly bump on this PR — happy to make changes or close it out if it is no longer useful. |
Member
|
Fixed by #45044, closing |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes #44792 - Handles None values in Janus model's image generation mode.
The
generate()method for image generation had several places where it assumed certain config values would always be set, causing failures when they were None:generation_config.num_return_sequencescould be None, causing errors in_expand_inputs_for_generation- now defaults to 1generation_config.generation_kwargs["boi_token_id"]was accessed without checking ifgeneration_kwargsexisted - now safely initializedgeneration_config.pad_token_idcould be None - now falls back to config valuemax_cache_lencalculation failed whenmax_lengthwas None - now properly computes a safe minimum cache length with bufferBefore submitting
Tagging: @zucchini-nlp for multimodal models