Skip to content

Update HF tests with default MC enabled, and remove skips#1009

Merged
dbasunag merged 2 commits intoopendatahub-io:mainfrom
dbasunag:undo_hf_skips
Jan 13, 2026
Merged

Update HF tests with default MC enabled, and remove skips#1009
dbasunag merged 2 commits intoopendatahub-io:mainfrom
dbasunag:undo_hf_skips

Conversation

@dbasunag
Copy link
Copy Markdown
Collaborator

@dbasunag dbasunag commented Jan 12, 2026

Description

How Has This Been Tested?

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • 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

Summary by CodeRabbit

  • Tests

    • Enabled previously skipped HuggingFace model tests for search, sorting, and validation.
    • Updated model exclusion test cases to reflect revised model filtering patterns.
  • Chores

    • Updated model catalog with new models added and model exclusion patterns refreshed.

✏️ Tip: You can customize this high-level summary in your review settings.

@github-actions
Copy link
Copy Markdown

The following are automatically added/executed:

  • PR size label.
  • Run pre-commit
  • Run tox
  • Add PR author as the PR assignee
  • Build image based on the PR

Available user actions:

  • To mark a PR as WIP, add /wip in a comment. To remove it from the PR comment /wip cancel to the PR.
  • To block merging of a PR, add /hold in a comment. To un-block merging of PR comment /hold cancel.
  • To mark a PR as approved, add /lgtm in a comment. To remove, add /lgtm cancel.
    lgtm label removed on each new commit push.
  • To mark PR as verified comment /verified to the PR, to un-verify comment /verified cancel to the PR.
    verified label removed on each new commit push.
  • To Cherry-pick a merged PR /cherry-pick <target_branch_name> to the PR. If <target_branch_name> is valid,
    and the current PR is merged, a cherry-picked PR would be created and linked to the current PR.
  • To build and push image to quay, add /build-push-pr-image in a comment. This would create an image with tag
    pr-<pr_number> to quay repository. This image tag, however would be deleted on PR merge or close action.
Supported labels

{'/build-push-pr-image', '/cherry-pick', '/hold', '/lgtm', '/wip', '/verified'}

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Jan 12, 2026

📝 Walkthrough

Walkthrough

This pull request updates the HuggingFace model catalog by replacing two model entries in the mixed models list, removes test-skip markers from four test files to enable test execution, and updates test assertions and expected model lists in the exclude models test to use subset checks instead of exact equality.

Changes

Cohort / File(s) Summary
Model Catalog Constants
tests/model_registry/model_catalog/constants.py
Added "microsoft/Phi-4-mini-reasoning" and "microsoft/Phi-3.5-mini-instruct"; removed "RedHatAI/phi-4-quantized.w8a8" and "RedHatAI/Qwen2.5-7B-Instruct" from HF_MODELS["mixed"] list.
Test Skip Marker Removals
tests/model_registry/model_catalog/huggingface/test_huggingface_model_search.py, test_huggingface_model_sorting.py, test_huggingface_model_validation.py
Removed module-level pytest skip markers to enable test execution in three test files.
HuggingFace Model Exclusion Tests
tests/model_registry/model_catalog/huggingface/test_huggingface_exclude_models.py
Removed skip marker, updated excluded_models prefixes from RedHatAI/* to meta-llama/* and ibm-granite/*, updated expected model lists to include new microsoft models, and changed assertion logic from exact equality (==) to subset check (issubset).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main changes: enabling default MC in HF tests and removing skip markers from multiple test files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c4fef85 and 0b638b5.

📒 Files selected for processing (5)
  • tests/model_registry/model_catalog/constants.py
  • tests/model_registry/model_catalog/huggingface/test_huggingface_exclude_models.py
  • tests/model_registry/model_catalog/huggingface/test_huggingface_model_search.py
  • tests/model_registry/model_catalog/huggingface/test_huggingface_model_sorting.py
  • tests/model_registry/model_catalog/huggingface/test_huggingface_model_validation.py
💤 Files with no reviewable changes (3)
  • tests/model_registry/model_catalog/huggingface/test_huggingface_model_search.py
  • tests/model_registry/model_catalog/huggingface/test_huggingface_model_sorting.py
  • tests/model_registry/model_catalog/huggingface/test_huggingface_model_validation.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/model_registry/model_catalog/huggingface/test_huggingface_exclude_models.py (1)
tests/model_registry/model_catalog/utils.py (1)
  • get_hf_catalog_str (193-235)
🔇 Additional comments (4)
tests/model_registry/model_catalog/constants.py (1)

52-53: LGTM!

The model list updates are consistent with the PR objective to update HF tests with default MC enabled. The new Microsoft Phi model entries follow the existing naming convention.

tests/model_registry/model_catalog/huggingface/test_huggingface_exclude_models.py (3)

18-26: LGTM!

The updated test parameters correctly reflect the new model list from constants.py. With HF_MODELS["mixed"] now containing Microsoft and meta-llama models alongside ibm-granite, excluding ["meta-llama/*", "ibm-granite/*"] correctly leaves the three Microsoft models as expected.


36-50: LGTM!

The expected models for the specific exclusion test case are correctly updated to reflect the new model list. The xfail marker is appropriately retained for the known issue.


80-82: Verify the intentional relaxation of the assertion.

The change from exact equality to subset check means the test now only verifies that expected models are present, but no longer validates that only those models appear. This could allow unexpected models from other sources to pass undetected.

If this is intentional (e.g., default MC enabled may add models from other catalogs), consider documenting this behavior with a comment. Otherwise, you might want to add a check that len(catalog_model_names) == len(expected_models) to ensure no extra models appear.


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 and usage tips.

@dbasunag dbasunag enabled auto-merge (squash) January 13, 2026 17:54
Copy link
Copy Markdown
Contributor

@fege fege left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Copy Markdown
Contributor

@sheltoncyril sheltoncyril left a comment

Choose a reason for hiding this comment

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

/lgtm

@dbasunag dbasunag merged commit bd84bf5 into opendatahub-io:main Jan 13, 2026
9 checks passed
@dbasunag dbasunag deleted the undo_hf_skips branch January 13, 2026 18:31
@github-actions
Copy link
Copy Markdown

Status of building tag latest: success.
Status of pushing tag latest to image registry: failure.

@github-actions
Copy link
Copy Markdown

Status of building tag latest: success.
Status of pushing tag latest to image registry: success.

mwaykole pushed a commit to mwaykole/opendatahub-tests that referenced this pull request Jan 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants