Skip to content

feat: general include exclude models field in sources file#1885

Merged
google-oss-prow[bot] merged 6 commits intokubeflow:mainfrom
Al-Pragliola:al-pragliola-unify-inclusion-exclusion
Nov 21, 2025
Merged

feat: general include exclude models field in sources file#1885
google-oss-prow[bot] merged 6 commits intokubeflow:mainfrom
Al-Pragliola:al-pragliola-unify-inclusion-exclusion

Conversation

@Al-Pragliola
Copy link
Copy Markdown
Contributor

@Al-Pragliola Al-Pragliola commented Nov 19, 2025 •
edited
Loading

Description

This PR introduces standardized model inclusion/exclusion filtering that works consistently across all catalog source types (YAML, Hugging Face, etc.).

How Has This Been Tested?

local testing plus unit/integration tests

Merge criteria:

  • All the commits have been signed-off (To pass the DCO check)
  • The commits have meaningful messages
  • Automated tests are provided as part of the PR for major new functionalities; testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work.
  • Code changes follow the kubeflow contribution guidelines.
  • For first time contributors: Please reach out to the Reviewers to ensure all tests are being run, ensuring the label ok-to-test has been added to the PR.

@google-oss-prow google-oss-prow Bot requested review from jonburdo and pboyd November 19, 2025 18:00
@google-oss-prow google-oss-prow Bot added size/XL and removed size/L labels Nov 20, 2025
@Al-Pragliola Al-Pragliola marked this pull request as ready for review November 20, 2025 17:03
pboyd
pboyd approved these changes Nov 20, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@pboyd pboyd left a comment

Choose a reason for hiding this comment

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

The this looks really good. I was only expecting a prefix match, but your regex solution is much better.

/lgtm

Comment thread catalog/internal/catalog/yaml_catalog.go Outdated
Comment thread catalog/internal/catalog/model_filter.go Outdated
@google-oss-prow google-oss-prow Bot added lgtm and removed lgtm labels Nov 20, 2025
Al-Pragliola and others added 5 commits November 20, 2025 20:23
@Al-Pragliola
feat: general include exclude models field in sources file
b6966d7 
Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>
@Al-Pragliola
chore: refactor source filter validation
cd6a614 
Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>
@Al-Pragliola
chore: refactor validation function
cb3c16e 
Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>
@Al-Pragliola @pboyd
feat: apply suggestions from code review
e6be598 
Co-authored-by: Paul Boyd <paul@pboyd.io>
Signed-off-by: Alessio Pragliola <83355398+Al-Pragliola@users.noreply.github.com>
@Al-Pragliola
chore: add case insensitive test
3958a5b 
Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>
@Al-Pragliola Al-Pragliola force-pushed the al-pragliola-unify-inclusion-exclusion branch from 6ef9c6e to 3958a5b Compare November 20, 2025 19:23
jonburdo
jonburdo approved these changes Nov 20, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@jonburdo jonburdo left a comment

Choose a reason for hiding this comment

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

/lgtm as long as we do indeed want this to be case-insensitive

Comment thread api/openapi/catalog.yaml
Comment on lines +507 to +509
Optional allow-list of models that are eligible for this source. Entries can be
exact model names or patterns that use `*` as a wildcard. When provided, only
models matching at least one pattern are considered.
Copy link
Copy Markdown
Member

@jonburdo jonburdo Nov 20, 2025

Choose a reason for hiding this comment

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

To confirm, we want this to be case-insensitive? Maybe worth mentioning that here. I would generally assume a glob is case-sensitive like it would be in a shell.

Copy link
Copy Markdown
Contributor Author

@Al-Pragliola Al-Pragliola Nov 20, 2025

Choose a reason for hiding this comment

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

yeah making it case insensitive makes totally sense in my opinion in our intended use case, users probably don't even know the exact naming of the models that are needed to be excluded/included or what a glob is

Copy link
Copy Markdown
Member

@jonburdo jonburdo Nov 20, 2025

Choose a reason for hiding this comment

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

Sounds good. We should just make sure to document that

Copy link
Copy Markdown
Contributor Author

@Al-Pragliola Al-Pragliola Nov 20, 2025

Choose a reason for hiding this comment

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

yes thank you @jonburdo , please check the latest commit c049854

@google-oss-prow google-oss-prow Bot added the lgtm label Nov 20, 2025
@Al-Pragliola
feat: add case-insensitive description
c049854 
Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>
@google-oss-prow google-oss-prow Bot removed the lgtm label Nov 20, 2025
jonburdo
jonburdo approved these changes Nov 20, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@jonburdo jonburdo left a comment

Choose a reason for hiding this comment

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

/lgtm

@google-oss-prow google-oss-prow Bot added the lgtm label Nov 20, 2025
@pboyd
Copy link
Copy Markdown
Member

pboyd commented Nov 21, 2025

/approve

@google-oss-prow
Copy link
Copy Markdown
Contributor

google-oss-prow Bot commented Nov 21, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pboyd

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow Bot merged commit fb2564f into kubeflow:main Nov 21, 2025
24 of 25 checks passed
@Al-Pragliola Al-Pragliola deleted the al-pragliola-unify-inclusion-exclusion branch November 21, 2025 12:50
This was referenced Nov 21, 2025
Merged
Merged
@pboyd pboyd mentioned this pull request Dec 1, 2025
Merged
8 tasks
@jonburdo jonburdo mentioned this pull request Jan 27, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pboyd pboyd pboyd approved these changes

@jonburdo jonburdo jonburdo approved these changes

Assignees

@pboyd pboyd

@jonburdo jonburdo

Labels

approved Area/Go REST server Area/Model Catalog lgtm size/XL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Al-Pragliola @pboyd @jonburdo