This is a nit, but I wonder if you could keep a list of length 1 here and repeat init_tokens batch_size times at the end of the method instead, WDYT ?

Sounds like a good idea to me.

init_tokens is only 1 long in the case of language=None at this point (because when you do the deepcopy on init_tokens, it assumes that a list of languages has been passsed in). We also need to set language for a later check in the function, since the dim of language is used in a check

To fix this, we can do:

id_to_lang = {v: k for k, v in generation_config.lang_to_id.items()}
language = [id_to_lang[lang_id.item()] for lang_id in lang_ids]
init_tokens = [copy.deepcopy(init_tokens[0]) for _ in lang_ids]

(the init tokens is a bit janky above, it would be cleaner if we didn't do the deepcopy above since then it is a list of lists).

you can validate the current failure with the test of multiple languages, but passing None as the language

True! But it's becoming a bit convoluted then. It's probably cleaner to revert 0b424f7 so that init_tokens and language are always of length batch_size. @ylacombe?

Not sure to follow the issue here, could you expand on this ?

@ylacombe 0b424f7 was based on your suggestion, but it turns out it broke language detection. So maybe we can revert that unless you think it would introduce significant overhead.

Another option would be to do something like this:

        if isinstance(language, (list, tuple)):
            ...
        elif language is None:
            # Language will be detected for each item in batch
            language = [None] * batch_size
        else:
            language = [language]  # Use a length-1 list now, broadcast later

        # Separate init_tokens for each language
        init_tokens = [copy.deepcopy(init_tokens) for _ in language]

Actually @naveen-corpusant, I don't think you're right on your second point. That later check is not checking the detected language, it's there to check if language was passed to generate() and if so, set the task to transcription. I'll introduce a variable languages in order to keep the original language parameter intact.

I went with my suggestion above, so language will be a length-1 list if a single language was passed, otherwise it will be a length-batch_size list of either languages or Nones (if the languages need to be detected).

I would be in favour of moving this normalisation of the language argument to the _retrieve_init_tokens method. It can be executed just before we map the language string argument to the langauge id token:

@sanchit-gandhi But this is already the _retrieve_init_tokens function. But yes, I'll move it here to normalize the list of languages once it's already been ensured that it's a list.

Ah ok it's added back here. Could we move it back up so it sits next to the forced decoder id logic?

Is there a reason we need a deepcopy here?

Not really, will change it to copy then.

The rest code assumes that we're using the same init tokens across the entire batch, but in the proposed changes we're looping over each batch item and computing the init tokens for each. This is redundant since the init tokens will be the same for each element of the batch. In practice, we should only need to compute them once!

Instead of doing this looping, can we keep the existing code and compute the init tokens just once? We can then copy them for each element in the batch, in a similar way to how you did previously:

        # Separate init_tokens for each language
        init_tokens = [copy.deepcopy(init_tokens) for _ in language]

Actually, init_tokens will either be length 1 (if a single language was passed), or have possibly different values for each element in the batch (if a batch of languages was passed or the languages were detected). So there is no redundancy.

One could maybe handle the task tokens first and only then expand init_tokens to the size of the batch (if needed), but with the way it's written, this is not easily possible (because the language token comes before the task token).

Sure - but the rest of the init tokens (start token id, task and timestamps) are fixed across the batch? So we only need to do a batched computation for the language token id (multiple ids), and for the rest we only need do it once for a single item, and then copy it for the rest of the batch elements?

The current version of the code makes it clear why we need to do this - happy with how it is! #29688 (comment)

Nice test! Would you mind defining a new test function for this test and moving the new checks into here? (e.g. test_large_generation_batched_languages) Keeping the tests small and modular helps identify which parts of the code are potentially broken with new PRs

Done!

I'm not sure why this function was used here. The language token is always going to be at init_tokens[1], so it's doing the same thing as this bit in the elif branch (only slower):

Done!

This looks much cleaner and I understand the motivation for looping over the init_tokens!