-
Notifications
You must be signed in to change notification settings - Fork 31.4k
[janus] Fix failing tests on mi3XX #38426
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
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
| # Get BOI token ID | ||
| if hasattr(generation_config, "generation_kwargs"): | ||
| boi_token_id = generation_config.generation_kwargs.get("boi_token_id", generation_config.bos_token_id) | ||
| else: | ||
| boi_token_id = kwargs.get("boi_token_id", generation_config.bos_token_id) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @remi-or , The other changes look logical to me, thanks for fixing them 🤗 . Can you expand on why boi_token_id won't be present in the generation_kwargs coz AFAIK I have added it explicitly in conversion file; hence should be present in checkpoints 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @yaswanth19 , it seems like it is missing from the checkpoint used in testing (deepseek-community/Janus-Pro-1B) .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see in the checkpoints: https://huggingface.co/deepseek-community/Janus-Pro-1B/blob/main/generation_config.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like generation_config is either 1. not loaded, which would be weird because guidance_scale is equal to 5 (but idk if that's usual) or 2. the generation_kwargs attribute is dropped at some point. Before model.generate is called in the test, if I check the value of model.generation_config I get:
model.generation_config = GenerationConfig {
"bos_token_id": 100000,
"eos_token_id": 100001,
"pad_token_id": 100002
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@remi-or do you have a snippet reproducing the issue, would be nice to add it in the PR body as well 🤗
I wonder if the issue you describe appears only on certain hardware (which is very unlikely) or the inference script is doing smth unexpected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! It's taken from tests/models/janus/test_modeling_janus.py::JanusIntegrationTest::test_model_generate_image :
from transformers import AutoProcessor, JanusForConditionalGeneration
if __name__ == "__main__":
model_id = "deepseek-community/Janus-Pro-1B"
model = JanusForConditionalGeneration.from_pretrained(model_id, device_map="auto")
processor = AutoProcessor.from_pretrained(model_id)
inputs = processor(
text=["A portrait of young girl. masterpiece, film grained, best quality."],
padding=True, generation_mode="image", return_tensors="pt",
).to(model.device)
out = model.generate(**inputs, generation_mode="image", do_sample=False)
I tried changing device_map to CPU and it still crashed with AttributeError: 'GenerationConfig' object has no attribute 'generation_kwargs' so I dont think it's device-related.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, weird since I'd assume the config from the hub would be picked up. At least that was try for Whisper in the past. Let me check why this isn't loaded, we better make sure the pre-saved config values are used when running inference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, the issue was in the config saved in the hub. One of the flags was set to True thus overwriting config values from scratch
I think now the only issue is the multi-device inferece. @remi-or can you update the PR so we can merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
|
CC: @zucchini-nlp |
| # Get BOI token ID | ||
| if hasattr(generation_config, "generation_kwargs"): | ||
| boi_token_id = generation_config.generation_kwargs.get("boi_token_id", generation_config.bos_token_id) | ||
| else: | ||
| boi_token_id = kwargs.get("boi_token_id", generation_config.bos_token_id) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question
zucchini-nlp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great thanks!
* [janus] Fix failing tests on mi3XX (#38426) * Fix multiple devices error on Janus * Fix AttributeError on Janus BOI token * Initialize lm first in Janus to get correct device map * Added expectations for Janus test_model_generate_images * Fixed JanusVisionEncoderLayer being split across devices * Code formatting * Adding modeling file * Reverted changes out of scope for this PR * [seamless_m4t] Skip some tests when speech is not available (#38430) * Added the require_speech decorator * Added require_speecj to some seamless_m4t tests * Changed skip message
* Fix multiple devices error on Janus * Fix AttributeError on Janus BOI token * Initialize lm first in Janus to get correct device map * Added expectations for Janus test_model_generate_images * Fixed JanusVisionEncoderLayer being split across devices * Code formatting * Adding modeling file * Reverted changes out of scope for this PR
This PR fixes a few test fails of the
janusmodel on MI3XX:generation_kwargsattribute that is not guaranteed to be in the config;Expectations