[FEAT] add 'include_finished_set' parameter to scheduler config

[ivyilike]

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.

Purpose

To fix issue #22545, I exposed the include_finished_set parameter. When this parameter is enabled, a separate set of finished request ids will be included in the EngineCoreOutputs. This allows stats to be updated when all requests are aborted.

(Optional) Documentation Update

[github-actions]

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can either: Add ready label to the PR or enable auto-merge.

🚀

[gemini-code-assist]

Code Review

This pull request introduces a new include_finished_set parameter to the scheduler configuration. This allows for explicitly enabling the tracking of finished request IDs, which is useful for updating statistics when all requests are aborted, particularly in non-data-parallel scenarios. The changes are well-implemented, adding the new parameter to the SchedulerConfig, exposing it through EngineArgs and the CLI, and correctly updating the logic in the EngineCore to use this new flag. The implementation is clean and follows the existing codebase patterns. I have no major concerns with this change.

[hmellor]

Should this be fixed in the engine rather than being an opt-in always on feature?

[njhill]

Thanks @ivyilike. Like @hmellor said, we shouldn't need to introduce a new arg for this. I think the problem isn't that we aren't returning the finished request ids per se, but that we don't return the stats when there are no request outputs.

I've opened a PR to address that, still need to add a test to verify: #22995

Really appreciate you looking into this though, I'll add you as coauthor since this helped me to pinpoint the issue.

[ivyilike]

Thanks @njhill , indeed your PR can address this issue at its root. However, shouldn't we also expose this parameter particularly in non-data-parallel scenarios, to track the finished request IDs.

[njhill]

Thanks @njhill , indeed your PR can address this issue at its root. However, shouldn't we also expose this parameter particularly in non-data-parallel scenarios, to track the finished request IDs.

@ivyilike this is an internal interface so I don't think it makes sense for this to be configurable.

[njhill]

Closing this now since it's addressed by #22995.