fix: update automation to reflect the changes around new source for other models#1232
Merged
dbasunag merged 4 commits intoopendatahub-io:mainfrom Mar 17, 2026
Merged
Conversation
…ther models Signed-off-by: Debarati Basu-Nag <dbasunag@redhat.com>
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/wip', '/cherry-pick', '/lgtm', '/verified', '/build-push-pr-image', '/hold'} |
Contributor
📝 WalkthroughWalkthroughThis PR expands the test infrastructure to support an additional catalog (OTHER_MODELS_CATALOG_ID), raising the default catalog count from 2 to 3. It introduces source-based filtering by adding a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/model_registry/model_catalog/catalog_config/utils.py (1)
426-461: Docstring parameter order doesn't match function signature.Function signature has
source_id(line 430) beforeexpected_models(line 431), but docstring documentsexpected_models(line 440) beforesource_id(line 441). Update docstring to match actual parameter order.📝 Proposed docstring fix
""" Wait for specific model set to appear using `@retry` decorator. Args: model_catalog_rest_url: API URL list model_registry_rest_headers: API headers source_label: Source to query - expected_models: Expected set of model names source_id: Source to query + expected_models: Expected set of model names Returns: Set of matched models🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/model_catalog/catalog_config/utils.py` around lines 426 - 461, The docstring for wait_for_model_set_match lists the Args in the wrong order; update the Args section so parameter entries match the function signature order: model_catalog_rest_url, model_registry_rest_headers, source_label, source_id, expected_models (with their existing descriptions), ensuring source_id appears before expected_models to match the signature used in the function body.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/model_registry/model_catalog/constants.py`:
- Around line 40-46: The constant OTHER_MODELS_CATALOG_ID currently sets
"labels": None which causes AttributeError when other code (e.g., conftest.py
tests that call labels.append() and metadata/utils.py functions that call
labels.extend()) expects a list; update the constant OTHER_MODELS_CATALOG_ID to
use an empty list for "labels" instead of None so it matches the list-based
handling used by validate_default_catalog(), conftest.py, and metadata/utils.py.
---
Nitpick comments:
In `@tests/model_registry/model_catalog/catalog_config/utils.py`:
- Around line 426-461: The docstring for wait_for_model_set_match lists the Args
in the wrong order; update the Args section so parameter entries match the
function signature order: model_catalog_rest_url, model_registry_rest_headers,
source_label, source_id, expected_models (with their existing descriptions),
ensuring source_id appears before expected_models to match the signature used in
the function body.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: eb289dd1-4c2b-4064-85b9-5641b336a59d
📒 Files selected for processing (6)
tests/model_registry/model_catalog/catalog_config/test_default_model_catalog.pytests/model_registry/model_catalog/catalog_config/test_default_source_inclusion_exclusion_cleanup.pytests/model_registry/model_catalog/catalog_config/utils.pytests/model_registry/model_catalog/conftest.pytests/model_registry/model_catalog/constants.pytests/model_registry/model_catalog/metadata/test_sources_endpoint.py
Collaborator
Author
|
/build-push-pr-image |
|
Status of building tag pr-1232: success. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/model_registry/model_catalog/catalog_config/test_default_source_inclusion_exclusion_cleanup.py (1)
228-265:⚠️ Potential issue | 🟠 MajorPhase 2 mixes two different expected outcomes.
included_models=["*"]withexcluded_models=["*granite*"]should leave every non-granite model inREDHAT_AI_CATALOG_ID, but this block waits for and assertsprometheus_modelsonly. This only passes if that source happens to contain no other non-granite entries. If the intent is “prometheus only”, narrow the phase-2 patch instead of the expectation.Suggested fix
# Verify granite models are removed (cleanup behavior) try: + expected_remaining_models = baseline_redhat_ai_models["api_models"] - granite_models phase2_api_models = wait_for_model_set_match( model_catalog_rest_url=model_catalog_rest_url, model_registry_rest_headers=model_registry_rest_headers, source_label=REDHAT_AI_CATALOG_NAME, - expected_models=prometheus_models, + expected_models=expected_remaining_models, source_id=REDHAT_AI_CATALOG_ID, ) except TimeoutExpiredError as e: - pytest.fail(f"Phase 2: Timeout waiting for prometheus models {prometheus_models}: {e}") + pytest.fail(f"Phase 2: Timeout waiting for non-granite models {expected_remaining_models}: {e}") @@ - # Should only have prometheus models now - assert phase2_api_models == prometheus_models, ( - f"Phase 2: Expected only prometheus {prometheus_models}, got {phase2_api_models}" + assert phase2_api_models == expected_remaining_models, ( + f"Phase 2: Expected non-granite models {expected_remaining_models}, got {phase2_api_models}" ) - assert phase2_db_models == prometheus_models, "Phase 2: DB should match API" + assert phase2_db_models == expected_remaining_models, "Phase 2: DB should match API"As per coding guidelines,
**: REVIEW PRIORITIES: 3. Bug-prone patterns and error handling gaps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/model_catalog/catalog_config/test_default_source_inclusion_exclusion_cleanup.py` around lines 228 - 265, The test mixes a broad include with a narrow expectation: in the modify_catalog_source call (phase2_patch) you set included_models=["*"] and excluded_models=["*granite*"] but then call wait_for_model_set_match and assert phase2_api_models == prometheus_models; update the test so the intent matches the patch — either narrow the phase2_patch included_models to target prometheus only (e.g., include pattern matching prometheus) or change the expectation to compute/verify the set of all non-granite models for REDHAT_AI_CATALOG_ID (use the same filtering logic used by modify_catalog_source) and assert phase2_api_models and phase2_db_models equal that non-granite set, and adjust the LOGGER message accordingly.
♻️ Duplicate comments (1)
tests/model_registry/model_catalog/constants.py (1)
40-45:⚠️ Potential issue | 🟠 MajorUse a list for
OTHER_MODELS_CATALOG_ID["labels"].The other
DEFAULT_CATALOGSentries use a list here, and the catalog update helpers mutatelabelswith list operations. Keeping this source atNoneturns the first append/extend into anAttributeError.Suggested fix
OTHER_MODELS_CATALOG_ID: { "name": OTHER_MODELS, "type": "yaml", "properties": {"yamlCatalogPath": "/shared-data/other-models-catalog.yaml"}, - "labels": None, + "labels": [], "enabled": True, },As per coding guidelines,
**: REVIEW PRIORITIES: 3. Bug-prone patterns and error handling gaps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/model_catalog/constants.py` around lines 40 - 45, The OTHER_MODELS_CATALOG_ID entry in DEFAULT_CATALOGS sets "labels" to None which causes an AttributeError when catalog update helpers perform list mutations; change the "labels" value for OTHER_MODELS_CATALOG_ID to an empty list ([]) so functions that call list methods (e.g., append/extend) on labels operate safely; locate the dictionary named OTHER_MODELS_CATALOG_ID in tests/model_registry/model_catalog/constants.py and replace the None labels value with an empty list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@tests/model_registry/model_catalog/catalog_config/test_default_source_inclusion_exclusion_cleanup.py`:
- Around line 228-265: The test mixes a broad include with a narrow expectation:
in the modify_catalog_source call (phase2_patch) you set included_models=["*"]
and excluded_models=["*granite*"] but then call wait_for_model_set_match and
assert phase2_api_models == prometheus_models; update the test so the intent
matches the patch — either narrow the phase2_patch included_models to target
prometheus only (e.g., include pattern matching prometheus) or change the
expectation to compute/verify the set of all non-granite models for
REDHAT_AI_CATALOG_ID (use the same filtering logic used by
modify_catalog_source) and assert phase2_api_models and phase2_db_models equal
that non-granite set, and adjust the LOGGER message accordingly.
---
Duplicate comments:
In `@tests/model_registry/model_catalog/constants.py`:
- Around line 40-45: The OTHER_MODELS_CATALOG_ID entry in DEFAULT_CATALOGS sets
"labels" to None which causes an AttributeError when catalog update helpers
perform list mutations; change the "labels" value for OTHER_MODELS_CATALOG_ID to
an empty list ([]) so functions that call list methods (e.g., append/extend) on
labels operate safely; locate the dictionary named OTHER_MODELS_CATALOG_ID in
tests/model_registry/model_catalog/constants.py and replace the None labels
value with an empty list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 182e69b2-8d79-4bb4-a4be-b55aa2afea65
📒 Files selected for processing (4)
tests/model_registry/model_catalog/catalog_config/test_default_model_catalog.pytests/model_registry/model_catalog/catalog_config/test_default_source_inclusion_exclusion_cleanup.pytests/model_registry/model_catalog/constants.pytests/model_registry/model_catalog/metadata/test_sources_endpoint.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/model_registry/model_catalog/catalog_config/test_default_model_catalog.py
- tests/model_registry/model_catalog/metadata/test_sources_endpoint.py
Contributor
|
/lgtm |
SB159
approved these changes
Mar 17, 2026
rhods-ci-bot
approved these changes
Mar 17, 2026
|
Status of building tag latest: success. |
ssaleem-rh
pushed a commit
to ssaleem-rh/opendatahub-tests
that referenced
this pull request
Mar 23, 2026
…ther models (opendatahub-io#1232) Signed-off-by: Debarati Basu-Nag <dbasunag@redhat.com> Signed-off-by: Shehan Saleem <ssaleem@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Summary
Tests needed to be readjusted due to introduction of a new source in default catalog configmap.
Related Issues
How it has been tested
Additional Requirements
Summary by CodeRabbit
New Features
Tests