Skip to content

Conversation

@njhill
Copy link
Member

@njhill njhill commented Oct 27, 2025

Currently just doing full CI test to flush out any issues.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request enables asynchronous scheduling by default, which is a significant and beneficial change for performance. The implementation involves refactoring the configuration handling to introduce a disable_async_scheduling flag and automatically disabling async scheduling for incompatible features like pipeline parallelism and speculative decoding. The core execution flow is also refactored into a two-step process (execute_model and sample_tokens) to support async structured outputs. The changes are extensive and touch many parts of the codebase, including configuration, core engine logic, executors, and tests.

My review has identified a critical issue with the configuration validation that could lead to unexpected hard failures instead of the intended automatic disabling of async scheduling. I've also found a potential issue in the KV output aggregation logic that might not handle None outputs from all workers correctly. I've provided suggestions to address these issues.

The rest of the changes, including test updates, seem correct and consistent with the goal of this PR.

@njhill njhill marked this pull request as draft October 27, 2025 23:10
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@njhill njhill force-pushed the async-sched-default branch from a193663 to b027710 Compare October 28, 2025 01:47
@njhill njhill marked this pull request as ready for review October 28, 2025 01:49
@njhill njhill force-pushed the async-sched-default branch 2 times, most recently from 70a0041 to b42e2d2 Compare November 12, 2025 23:14
@njhill njhill removed the ready ONLY add when PR is ready to merge/full CI is needed label Nov 13, 2025
@njhill njhill marked this pull request as draft November 13, 2025 18:03
@njhill njhill force-pushed the async-sched-default branch from b42e2d2 to 467aef5 Compare November 13, 2025 18:03
@njhill njhill marked this pull request as ready for review November 13, 2025 18:04
chatgpt-codex-connector[bot]

This comment was marked as outdated.

@njhill njhill added the ready ONLY add when PR is ready to merge/full CI is needed label Nov 13, 2025
@njhill njhill force-pushed the async-sched-default branch 4 times, most recently from 62677ea to 8487595 Compare November 14, 2025 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

frontend gpt-oss Related to GPT-OSS models kv-connector ready ONLY add when PR is ready to merge/full CI is needed structured-output suppress-bc-linter v1

Projects

Status: No status
Status: To Triage

Development

Successfully merging this pull request may close these issues.

4 participants