Skip to content

[diffusion] fix: handle disabled component residency in executor#24748

Closed
mickqian wants to merge 1 commit into
mainfrom
diffusion-fix-component-resident-manager
Closed

[diffusion] fix: handle disabled component residency in executor#24748
mickqian wants to merge 1 commit into
mainfrom
diffusion-fix-component-resident-manager

Conversation

@mickqian
Copy link
Copy Markdown
Collaborator

@mickqian mickqian commented May 9, 2026

Motivation

Modifications

Accuracy Tests

Speed Tests and Profiling

Checklist

Review and Merge Process

  1. Ping Merge Oncalls to start the process. See the PR Merge Process.
  2. Get approvals from CODEOWNERS and other reviewers.
  3. Trigger CI tests with comments or contact authorized users to do so.
    • Common commands include /tag-and-rerun-ci, /tag-run-ci-label, /rerun-failed-ci
  4. After green CI and required approvals, ask Merge Oncalls or people with Write permission to merge the PR.

@mickqian
Copy link
Copy Markdown
Collaborator Author

mickqian commented May 9, 2026

/tag-and-rerun-ci

@github-actions github-actions Bot added the diffusion SGLang Diffusion label May 9, 2026
@mickqian mickqian marked this pull request as ready for review May 9, 2026 03:00
@github-actions github-actions Bot added the run-ci label May 9, 2026
Copy link
Copy Markdown
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 introduces null checks for component_residency_manager in the PipelineExecutor class to prevent potential runtime errors. The review feedback identifies a potential AttributeError in begin_component_residency_request when the batch parameter is a list during grouped execution and suggests handling this case in both begin_component_residency_request and before_stage to ensure type consistency.

Comment on lines +56 to +57
if self.component_residency_manager is None:
return
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

While adding the null check is correct, the batch parameter can be a list of requests when called from execute_group. The component_residency_manager.begin_request method expects a single object satisfying the ResidencyBatch protocol (to access is_warmup), so passing a list will cause an AttributeError. Rebinding batch to its first element when it is a list ensures compatibility with grouped execution.

Suggested change
if self.component_residency_manager is None:
return
if self.component_residency_manager is None:
return
if isinstance(batch, list):
batch = batch[0]

Comment on lines +68 to +69
if self.component_residency_manager is None:
return
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Similar to begin_component_residency_request, batch can be a list of requests during grouped execution. Although the current implementation of manager.before_stage does not access batch.is_warmup, it is better to pass the expected type to maintain consistency and avoid potential issues if the manager's implementation changes in the future.

        if self.component_residency_manager is None:
            return
        if isinstance(batch, list):
            batch = batch[0]

@gemini-code-assist
Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

diffusion SGLang Diffusion run-ci

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant