Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't try to schedule EmbeddingFwdOp #4023

Merged
merged 1 commit into from
Mar 6, 2025

Conversation

naoyam
Copy link
Collaborator

@naoyam naoyam commented Mar 6, 2025

This should also work around #4013

@naoyam naoyam requested a review from Priya2698 March 6, 2025 16:36
Copy link

github-actions bot commented Mar 6, 2025

Description

  • Updated scheduler to reject EmbeddingFwdOp in non-ExprEval schedulers

  • Improved error message for unsupported operations


Changes walkthrough 📝

Relevant files
Enhancement
registry.cpp
Reject EmbeddingFwdOp in non-ExprEval schedulers                 

csrc/scheduler/registry.cpp

  • Added EmbeddingFwdOp to the list of unsupported operations
  • Updated error message to indicate presence of unsupported operations
  • +3/-3     

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 No relevant tests
    ⚡ Recommended focus areas for review

    Redundant Comment

    The comment "These ops are are only accepted in ExprEval" contains a typo and is redundant as it is followed by a similar comment.

    // These ops are  are only accepted in `ExprEval`
    // scheduler, all other schedulers should reject them.
    Typo

    The comment contains a typo: "These ops are are only accepted in ExprEval" should be "These ops are only accepted in ExprEval".

    // These ops are  are only accepted in `ExprEval`
    Inconsistent Error Message

    The error message in scheduler_debug_utils::canScheduleRejectReason has changed from "SdpaOps are not supported." to "Has unsupported ops". Ensure that the new message is consistent with the project's error messaging guidelines.

    scheduler_type, "Has unsupported ops");

    @naoyam
    Copy link
    Collaborator Author

    naoyam commented Mar 6, 2025

    !build

    @naoyam naoyam merged commit eedc374 into main Mar 6, 2025
    16 checks passed
    @naoyam naoyam deleted the dont_try_to_schedule_embedding_fwd branch March 6, 2025 17:39
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    None yet
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants