If we default to something, should we default to model.config.max_position_embeddings instead? I would prefer no default at all, to be honest, the maximum length is often highly application-specific and a bad number is quite impactful (incorrect model behaviour and/or wasted memory)

For instance, 4096 is too large for some models (e.g., will start generating gibberish after 2k tokens) and too small for some use cases (like processing documents)

model.config.max_position_embeddings is wayyyy too big for some models IMO, we'll waste so much we could OOM on cache init

Many models (even super small ones) have more than 100k https://huggingface.co/meta-llama/Llama-3.2-1B/blob/main/config.json

I agree, and that's why I would prefer to have no defaults 😢 I don't think there is a good value that works on 99% of the cases

A missing value with a good exception is trivial to fix. A bad default is a pain to find.

No default == crash is a no no IMO 😉
We can check if it has eot -> will stop generating itself, if not we use a good default for the number of parameter, something simple like 7B -> 512 since its fast
14B -> 256
etc just to have not too much time spend but still generate a bit!

If we default to something, should we default to model.config.max_position_embeddings instead? I would prefer no default at all, to be honest, the maximum length is often highly application-specific and a bad number is quite impactful (incorrect model behaviour and/or wasted memory)

I think to add onto this, not only is a it quite impactful, it's also very tricky to debug the further down you go in the stack, especially in ExecuTorch running a model. As @gante said, max_seq_len is one of, if not the most important export variables, so I think it's good to clear confusion about this value early on by erroring out instead of setting an arbitrary default value that is hard to find.

Yep we need to either make the default value very very easy to find, or error out. Weighting both in both contexts

Alright, from this discussion it looks like more people are pro-erroring-out. WDYT about making them arguments of the exportable class though to simplify? Instead if having to set model.generation_config.cache_config which is a bit awkward

Following this discussion, I removed the defaults and added the batch_size, max_cache_len and device as args. In order to not break the current API, if those arguments are not passed, we check the cache_config for corresponding values, and error-out if they are not found.