Skip to content

Order overlapping regex targets by specificity - #852

Open
gyanu2507 wants to merge 2 commits into
vllm-project:mainfrom
gyanu2507:fix/match-targets-regex-specificity
Open

Order overlapping regex targets by specificity#852
gyanu2507 wants to merge 2 commits into
vllm-project:mainfrom
gyanu2507:fix/match-targets-regex-specificity

Conversation

@gyanu2507

Copy link
Copy Markdown

match_targets is documented as returning matches from most specific to least specific, and _scheme_from_targets just takes targets[0]. For two re: patterns the sort key fell through to lexicographic order of the pattern strings, so a short catch-all like re:.*proj could beat re:.*q_proj and the layer got the wrong scheme.

Exact names still come first. Regex matches are ordered by pattern length (longer first), then class-name matches as before.

Fixes #847

@coderabbitai

coderabbitai Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: d0709c9d-89c8-418f-a9b4-52ac6bfd6fee

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@mergify

mergify Bot commented Aug 23, 2026

Copy link
Copy Markdown

Merge Protections

🔴 1 of 1 protections blocking · waiting on 👀 reviews

Protection Waiting on
🔴 Require one maintainer review 👀 reviews

🔴 Require one maintainer review

Waiting for any of

  • approved-reviews-by=HDCharles
  • approved-reviews-by=brian-dellabetta
  • approved-reviews-by=dsikka
  • approved-reviews-by=kylesayrs
This rule is failing.

All PRs must have at least one approving review from a maintainer before merging.

  • any of:
    • approved-reviews-by=HDCharles
    • approved-reviews-by=brian-dellabetta
    • approved-reviews-by=dsikka
    • approved-reviews-by=kylesayrs
  • #changes-requested-reviews-by = 0

@mergify

mergify Bot commented Aug 23, 2026

Copy link
Copy Markdown

The quality checks have failed. Please run make style and make quality under
the root directory to adddress the lint failures. You will need to install the
dev optional install to get the required linting packages.

match_targets claimed most-specific-first ordering, but two re: patterns were sorted lexicographically so a short catch-all could win.

Signed-off-by: Gyanu <gyanum.ug20.cse@nitp.ac.in>
@gyanu2507
gyanu2507 force-pushed the fix/match-targets-regex-specificity branch from b95d5d1 to 4205865 Compare August 23, 2026 07:44
@mergify mergify Bot removed the quality-failed label Aug 23, 2026
@gyanu2507

Copy link
Copy Markdown
Author

Mergify Merge Protections is still blocking — looks like an approvals / rule gate rather than a test failure on the specificity ordering change.

@gyanu2507

Copy link
Copy Markdown
Author

Quality and DCO are green. The remaining red check is Mergify Merge Protections requiring one maintainer review. Ready whenever someone has a moment to look.

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.

match_targets orders overlapping regex targets lexicographically, not by specificity

1 participant