[Bugfix] when use s3 model cannot use default load_format

[lengrongfu]

Purpose

#23236 (comment)

Current cannot use default loader model from s3 , so should to check it.

Test Plan

Test Result

---

Essential Elements of an Effective PR Description Checklist

• The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
• The test plan, such as providing test command.
• The test results, such as pasting the results comparison before and after, or e2e results
• (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
• (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

[gemini-code-assist]

Code Review

This pull request aims to fix an issue where loading models from S3 fails with the default load_format. The change introduces a check in EngineArgs.__post_init__ to enforce that S3 models use the runai_streamer load format.

However, the current implementation has some critical issues. It is overly restrictive and will cause an error on the common use case of providing an S3 path with the default load_format="auto", leading to a poor user experience. Additionally, this bugfix lacks any unit tests, which is essential for verifying the correctness of the new logic and preventing regressions. I have provided a detailed comment with a suggested code change to improve the logic and a request to add comprehensive tests. Addressing these points is critical to ensure the change is robust and user-friendly.

[22quinn]

The failure would occur for other formats too if user does not set the right one. Do we want to clutter arg_utils with many custom overrides? Is the expectation for users to set it right or for vllm to always infer the right format? If we decide the 2nd way, we should infer everything.
We can get some second opinions. cc @hmellor

[lengrongfu]

The failure would occur for other formats too if user does not set the right one. Do we want to clutter arg_utils with many custom overrides? Is the expectation for users to set it right or for vllm to always infer the right format? If we decide the 2nd way, we should infer everything. We can get some second opinions. cc @hmellor

This is a very good point.

• From a user-friendly perspective, it's probably better for VLLM to always infer the correct format.
• From a software maintenance perspective, it's probably better for users to set it correctly.

So it's a trade-off.

[hmellor]

Instead of arg_utils we could move the validation into ModelConfig? This is then localised to where it is most relevant.

(FYI, currently ModelConfig lives in config/__init__.py but it will eventually by moved to config/model.py.)

[hmellor]

As for the design decision, if we have a value called "auto" it'd be nice it it worked for all the supported load_formats

[lengrongfu]

Instead of arg_utils we could move the validation into ModelConfig? This is then localised to where it is most relevant.

(FYI, currently ModelConfig lives in config/__init__.py but it will eventually by moved to config/model.py.)

1. because load_format field is in EngineArgs struct, canot access in ModelConfig; base before have like logic in create_model_config method, so i move to this method.
2. current don't config/model.py this file, i can commit a pr moved to config/model.py.

[DarkLight1337]

Closing as superseded by #23845

[lengrongfu]

@DarkLight1337 I'll verify whether this pull request #23845 solves my problem. If not, this pull request may still be needed.

[lengrongfu]

@DarkLight1337 After testing, I think this problem is not solved. #23845 this PR is like pr #23842. we can reopen this pr?

[lengrongfu]

Hey @hmellor @22quinn the PR has been updated. Could you please review it when you have a moment? Thanks!

[hmellor]

1. because load_format field is in EngineArgs struct, canot access in ModelConfig; base before have like logic in create_model_config method, so i move to this method.

load_format is a Literal not a struct. I made a mistake before, load_format is part of LoadConfig. Since this validation requires information about load_config and model_config can it be moved to VllmConfig instead?

[lengrongfu]

1. because load_format field is in EngineArgs struct, canot access in ModelConfig; base before have like logic in create_model_config method, so i move to this method.

load_format is a Literal not a struct. I made a mistake before, load_format is part of LoadConfig. Since this validation requires information about load_config and model_config can it be moved to VllmConfig instead?

@hmellor yes, it need load_config and model_config, so i add this validation to VllmConfig.try_verify_and_update_config .

[22quinn]

Added logging to avoid silent overrides. auto makes some sense and there's already a bunch of load_format override logic.