Reorganize HF tests and add filter by name tests for HF models#986
Reorganize HF tests and add filter by name tests for HF models#986dbasunag merged 2 commits intoopendatahub-io:mainfrom
Conversation
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/hold', '/wip', '/cherry-pick', '/build-push-pr-image', '/lgtm', '/verified'} |
📝 WalkthroughWalkthroughThis change refactors test organization by extracting specialized test classes into dedicated modules: HuggingFace model search and sorting tests are created in new files, artifact search tests are moved from the general test_model_search.py to a dedicated test_model_artifact_search.py file, and corresponding imports are cleaned up. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In
@tests/model_registry/model_catalog/huggingface/test_huggingface_model_search.py:
- Around line 46-47: The test indexes result["items"][0] without ensuring items
is non-empty; add a bounds check (e.g., assert result.get("items") and
len(result["items"]) > 0 or assert result.get("size", 0) > 0) before accessing
the first element so the assertion hf_model_name == result["items"][0]["name"]
cannot raise IndexError and provides a clear failure message if no items are
returned.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/model_registry/model_catalog/huggingface/test_huggingface_model_search.pytests/model_registry/model_catalog/huggingface/test_huggingface_model_sorting.pytests/model_registry/model_catalog/search/test_model_artifact_search.pytests/model_registry/model_catalog/search/test_model_search.pytests/model_registry/model_catalog/sorting/test_model_sorting.py
💤 Files with no reviewable changes (1)
- tests/model_registry/model_catalog/search/test_model_search.py
🧰 Additional context used
🧬 Code graph analysis (4)
tests/model_registry/model_catalog/huggingface/test_huggingface_model_search.py (3)
tests/model_registry/model_catalog/utils.py (2)
get_hf_catalog_str(632-674)get_models_from_catalog_api(504-557)tests/model_registry/model_catalog/conftest.py (1)
updated_catalog_config_map(103-124)tests/model_registry/conftest.py (2)
model_catalog_rest_url(641-650)model_registry_rest_headers(325-326)
tests/model_registry/model_catalog/search/test_model_artifact_search.py (2)
tests/model_registry/model_catalog/conftest.py (1)
randomly_picked_model_from_catalog_api_by_source(204-257)tests/model_registry/model_catalog/search/utils.py (1)
fetch_all_artifacts_with_dynamic_paging(261-292)
tests/model_registry/model_catalog/huggingface/test_huggingface_model_sorting.py (3)
tests/model_registry/model_catalog/sorting/utils.py (1)
assert_model_sorting(409-424)tests/model_registry/model_catalog/utils.py (1)
get_hf_catalog_str(632-674)tests/model_registry/conftest.py (2)
model_catalog_rest_url(641-650)model_registry_rest_headers(325-326)
tests/model_registry/model_catalog/sorting/test_model_sorting.py (1)
tests/model_registry/model_catalog/utils.py (1)
get_models_from_catalog_api(504-557)
🪛 Ruff (0.14.10)
tests/model_registry/model_catalog/huggingface/test_huggingface_model_search.py
29-29: Unused method argument: updated_catalog_config_map
(ARG002)
tests/model_registry/model_catalog/search/test_model_artifact_search.py
296-296: Unused method argument: enabled_model_catalog_config_map
(ARG002)
362-362: Unused method argument: enabled_model_catalog_config_map
(ARG002)
401-401: Unused method argument: enabled_model_catalog_config_map
(ARG002)
tests/model_registry/model_catalog/huggingface/test_huggingface_model_sorting.py
40-40: Unused method argument: updated_catalog_config_map
(ARG002)
🔇 Additional comments (6)
tests/model_registry/model_catalog/sorting/test_model_sorting.py (1)
5-5: LGTM! Clean refactoring.The removal of the
get_hf_catalog_strimport aligns with the extraction of HuggingFace-specific tests to a dedicated module. The import cleanup is correct since this utility is no longer referenced in this file.tests/model_registry/model_catalog/huggingface/test_huggingface_model_search.py (1)
29-29: False positive from static analysis.The
updated_catalog_config_mapparameter is required for the fixture's side effects (configuring the catalog with HuggingFace sources). This is a standard pytest pattern where fixtures are used for setup even when not directly referenced in the test body.tests/model_registry/model_catalog/huggingface/test_huggingface_model_sorting.py (1)
1-49: LGTM! Well-structured HuggingFace sorting tests.The test follows established patterns from the original
test_model_sorting.pyand provides comprehensive coverage of sorting functionality across all standard fields (ID, NAME, CREATE_TIME, LAST_UPDATE_TIME) in both directions. The reuse of theassert_model_sortingutility maintains consistency across the test suite.Note: The static analysis warning about unused
updated_catalog_config_map(line 40) is a false positive—the fixture is required for its side effects (catalog configuration setup).tests/model_registry/model_catalog/search/test_model_artifact_search.py (3)
277-348: LGTM! Thorough artifact type validation.The test validates artifact type filtering comprehensively:
- Fetches both complete and filtered artifact sets with dynamic paging
- Creates a lookup structure for efficient validation
- Verifies each filtered artifact exists in the complete set using
dictdiffer.difffor exact matching- Confirms no artifacts of the specified type were missed by the filter
The two-way validation (existence + completeness) ensures the filter returns exactly the expected artifacts.
Note: The static analysis warning about unused
enabled_model_catalog_config_map(line 296) is a false positive—the fixture is required for catalog setup.
360-387: LGTM! Proper error handling validation.The test correctly verifies that invalid artifact types raise
ResourceNotFoundErrorwith the expected error message format. This ensures API consumers receive clear, actionable error messages.Note: The static analysis warning about unused
enabled_model_catalog_config_map(line 362) is a false positive—the fixture is required for catalog setup.
399-433: LGTM! Multi-type filtering validation.The test validates that filtering by multiple artifact types (METRICS and MODEL) returns the complete set of artifacts for a model, confirming that the comma-separated
artifactTypeparameter works as expected.Note: The static analysis warning about unused
enabled_model_catalog_config_map(line 401) is a false positive—the fixture is required for catalog setup.
|
Status of building tag latest: success. |
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.