@zucchini-nlp can you take a look at this?

@zucchini-nlp
Hey Raushan!

I noticed that this test fails locally for me. Are you able to run it successfully on your end?

While reviewing the test, I saw that it calls generate twice and validates the results. In the second call, max_new_tokens isn’t explicitly provided, which leads to the test failing because the number of generated tokens exceeds 20. Since max_new_tokens isn’t set in this case, I believe the assertion should be adjusted to reflect the behavior when no explicit limit is applied. After making this change, the test passes locally for me.

Additionally, I added a third call to generate to verify that using max_new_tokens without specifying min_new_tokens works as expected. For this, I used an arbitrary value of 7 tokens for max_new_tokens. I believe this is a legitimate choice, given that the test already uses the value of 20 tokens arbitrarily in the first two calls without providing any deep explanation. The aim here is to ensure that the behavior remains consistent across different configurations and arbitrary limits.

Let me know if this makes sense or if you have any concerns. Would appreciate your approval if it looks alright to you.

@zucchini-nlp

weird it has not been causing us troubles

see #34807. TL;DR: test fetcher issue so some tests are not fetched (including this one)

Thank you @keyboardAnt for helping and this PR.

For this length issue, @gante in our team has opened #34814, but we are still discussing what would be the final call. Therefore I think it would be good not to merge this PR and wait for #34814 to make a decision.

@zucchini-nlp

weird it has not been causing us troubles

see #34807. TL;DR: test fetcher issue so some tests are not fetched (including this one)

Thank you @keyboardAnt for helping and this PR.

For this length issue, @gante in our team has opened #34814, but we are still discussing what would be the final call. Therefore I think it would be good not to merge this PR and wait for #34814 to make a decision.

@ydshieh, thank you for responding! I’m a bit puzzled though—since #34814 doesn’t fix test_generated_length_assisted_generation and this PR does, wouldn’t it make sense to merge this PR now to resolve the bug immediately? #34814 can still later refine or build on it as needed. Why leave the bug open when there’s already a fix ready to merge?

@keyboardAnt it should fix it by removing extra length added in utils.py but the discussion is still going on whether to keep that extra length or not. So yeah, let's wait

Hi @keyboardAnt . IIRC, the issue comes from #34377 which is from #34026 (and later #34617 is involved). This is more of the design we have to make decision, which is still under discussion. Fixing a test while we still need to agree on a choice isn't a super great idea :-).

(if it's about fixing something that affects many users' usage, that's another story)

But if our assumption is wrong (the causes of the failure), please correct us🙏