Rename max_length parameter to max_model_len to be in sync with vLLM

[Datta0]

What this PR does / why we need it:
vLLM uses parameter called max-model-len to denote the max length allowed for tokenisation and processing. We have similar parameter for HF case but under a diff name called max_length. Rename that to use the same parameter for both backends for consistency.
Now we're adding --max_model_len which would essentially write to args.max_model_len. We'll slowly move away from --max_length .

Checklist:

• Have you added unit/e2e tests that prove your fix is effective or that this feature works?
• Has code been commented, particularly in hard-to-understand areas?
• Have you made corresponding changes to the documentation?

Add `--max_model_len` to huggingface runtime and deprecate `--max_length`

[sivanantha321]

All the other args are using _(underscore). We should be consistent with the naming.

[Datta0]

vLLM uses - hyphenated separation. Using the same to ensure we can make use of only single parameter name.

[sivanantha321]

Wouldn't be a breaking change for current users ?

[Datta0]

Yeah this would be a breaking change for existing users. Should we allow both for time being and warn that max_length would be deprecated in favour of max-model-len while giving the latter precedence?
I'm curious if they're internally handling the setting of parameters with diff name for diff backends.

[sivanantha321]

@yuzisun What's your thought on this ?

[Datta0]

@yuzisun thoughts on this?

[spolti]

this should be possible:

arg_parser.add_argument('--example-one', '--example_one')

[Datta0]

So I just added max_length back which internally sets args.max_model_len.
Let me know if this works

[Datta0]

@yuzisun @sivanantha321 @spolti please take a look

[yuzisun]

Can we support both args.max_model_len and args.max-model-len and deprecate max_length ?

[Datta0]

/rerun-all

[yuzisun]

Suggested change

	"--max-model-len",
	"--max-model-len",
	"--max_model_len",

[Datta0]

We're just trying to have the same argument as vLLM. Hence the hyphen - instead of underscore _

[Datta0]

Also the problem with adding --max_model_len here is, this would not work if vLLM exists.
If we want to support --max_model_len, we'd need to add it separately. This ends up creating 3 CLI flags for one variable.

[yuzisun]

I thought --max-model-len is parsed to max_model_len as well

[Datta0]

Yeah --max-model-len is parsed to args.max_model_len
Just that if we want to support it, it should be added outside of if _vllm:

[yuzisun]

parser.add_argument('--foo-bar')
parser.add_argument('--foo_bar')
args = parser.parse_args(['--foo-bar', '24', '--foo_bar', '30'])
Namespace(foo_bar='30')

[Datta0]

if vLLM exists, we import that parser and hence it won't let us add --max-model-len again
so the final code would end up looking like

parser.add_argument('--max_length', dest='max_model_len')
parser.add_argument('--max_model_len')
if not vllm_available():
    parser.add_argument('--max-model-len')

All three for essentially the same variable. Do we want this?

[yuzisun]

vllm actually implements a flexible arg parser so it can automatically convert underscore to dashes
https://github.com/vllm-project/vllm/blob/80162c44b1d1e59a2c10f65b6adb9b0407439b1f/vllm/utils.py#L1088

[yuzisun]

Suggested change

dest="max_model_len",

[Datta0]

This is to make sure we're writing to same argument and hence avoid doing something like
final_max_len = args.max_length or args.max_model_len

[yuzisun]

Suggested change

	help="max sequence length for the tokenizer. will be deprecated in favour of --max-model-length",
	help="max sequence length for the tokenizer. will be deprecated in favour of --max-model-len",

[yuzisun]

/lgtm
/approve