Better defaults for assisted generation #40976
Conversation
| self.logits_processor = [ | ||
| processor for processor in self.logits_processor if not isinstance(processor, MinLengthLogitsProcessor) | ||
| ] |
There was a problem hiding this comment.
Length is controlled by main model's generation loop, so we should just discard those on the assistant right? @gante
src/transformers/generation/utils.py
Outdated
| # Prefer a slightly higher temperature for the assistant when not explicitly provided | ||
| idx = next((i for i, p in enumerate(logits_processor) if isinstance(p, TemperatureLogitsWarper)), None) | ||
| temp_processor = logits_processor.pop(idx) if idx is not None else TemperatureLogitsWarper(temperature=1.0) | ||
|
|
||
| if assistant_temperature is None and temp_processor is not None and temp_processor.temperature < 1.5: | ||
| logger.warning_once( | ||
| f"The assistant's sampling temperature comes from main generation loop set to {temp_processor.temperature}," | ||
| "but speculative decoding benefits from slightly hotter candidate generation, (see #40976)so we are setting it" | ||
| "to 1.5. This should improve decoding speed in most cases. Use `assistant_temperature` to override this value." | ||
| ) | ||
| assistant_temperature = 1.5 | ||
|
|
||
| if assistant_temperature is not None: | ||
| logits_processor.insert(0, TemperatureLogitsWarper(temperature=assistant_temperature)) |
There was a problem hiding this comment.
1.5 seems the most balanced for now, subject to change if experiments show otherwise, I am still benchmarking more models.
|
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. |
| "assistant_model": assistant_model, | ||
| "streamer": streamer, | ||
| "assistant_temperature": kwargs.pop("assistant_temperature", None), | ||
| } | ||
| generation_mode_kwargs["synced_gpus"] = ( |
There was a problem hiding this comment.
no need to change generate signature!! it gets automatically forwarded.
In fact, we could remove assistant_model from the signature (👀 v5?) and all the decoding method-specific kwargs get automatically forwarded.
There was a problem hiding this comment.
no signature change, but it's still an argument (that should be documented)
In any case, I'd rather have it being controlled by assistant_model.generation_config.temperature, in AssistedCandidateGenerator.__init__ -- if it's the default value (1.0), or == main model temperature, then override.
| self.logits_processor = [ | ||
| processor for processor in self.logits_processor if not isinstance(processor, MinLengthLogitsProcessor) | ||
| ] |
| # Prefer a slightly higher temperature for the assistant when not explicitly provided | ||
| idx = next((i for i, p in enumerate(logits_processor) if isinstance(p, TemperatureLogitsWarper)), None) | ||
| temp_processor = logits_processor.pop(idx) if idx is not None else TemperatureLogitsWarper(temperature=1.0) | ||
|
|
||
| if assistant_temperature is None and temp_processor is not None and temp_processor.temperature < 1.5: | ||
| logger.warning_once( | ||
| f"The assistant's sampling temperature comes from main generation loop set to {temp_processor.temperature}, " | ||
| "but speculative decoding benefits from slightly hotter candidate generation, (see #40976) so we are setting it " | ||
| "to 1.5. This should improve decoding speed in most cases. Use `assistant_temperature` to override this value." | ||
| ) | ||
| assistant_temperature = 1.5 | ||
|
|
||
| if assistant_temperature is not None: | ||
| logits_processor.insert(0, TemperatureLogitsWarper(temperature=assistant_temperature)) |
There was a problem hiding this comment.
doesn't this change the temperature for both models? 👀 (logits_processor also used in step 2.3)
| "assistant_model": assistant_model, | ||
| "streamer": streamer, | ||
| "assistant_temperature": kwargs.pop("assistant_temperature", None), | ||
| } | ||
| generation_mode_kwargs["synced_gpus"] = ( |
There was a problem hiding this comment.
no signature change, but it's still an argument (that should be documented)
In any case, I'd rather have it being controlled by assistant_model.generation_config.temperature, in AssistedCandidateGenerator.__init__ -- if it's the default value (1.0), or == main model temperature, then override.
#40657 inadvertently changed an implicit algorithmic bias: candidate_generator (the assistant model) was getting logits_processor while the decoding method (main model) was getting prepared_logits_processor. This meant that the assistant was running with T=1 while the main model was using lower temp.
We investigated and its good for speculation to have a hotter assistant model (so it was a good bug that we were not applying the lower temp to the assistant),
But it should be explicitly set and not a hidden argument forwarding consequence. This PR does that, setting it by default to 1.5.
This PR also fixes:
which originates from the same change in LogitsProcessor passing.