Skip to content

model validation automation v1#340

Merged
Raghul-M merged 37 commits intoopendatahub-io:mainfrom
edwardquarm:ed-model-validation
Jul 23, 2025
Merged

model validation automation v1#340
Raghul-M merged 37 commits intoopendatahub-io:mainfrom
edwardquarm:ed-model-validation

Conversation

@edwardquarm
Copy link
Copy Markdown
Contributor

@edwardquarm edwardquarm commented Jun 8, 2025

Description

Jira: RHOAIENG-27170
This PR is the first version of a test suite for automating the validation of modelcar OCI images using only vLLM runtimes.

Key features:

  • Parametrized test cases for validating multiple modelcar images in both raw and serverless deployment modes.
  • Support for YAML-based configuration as an alternative to CLI flags, improving flexibility.
  • CLI override support for: Runtime arguments, Supported accelerator type (e.g., Nvidia, AMD).
  • Registry pull secret injection for OCI-based model deployment.

Future plans:

  • Support for user-defined completion and chat query inputs.
  • Parallel execution of raw and serverless validations to improve test throughput.
  • Nested YAML configuration to select test categories (e.g., smoke, sanity, parity).

How Has This Been Tested?

Manually verified using both YAML and CLI-based test inputs. Example CLI usage:
set up your env variables

export OCI_REGISTRY_PULL_SECRET=
uv run pytest -vv tests/model_serving/model_runtime/model_validation/test_modelvalidation.py \
  --model_car_yaml_path=./tests/model_serving/model_runtime/model_validation/sample_modelcar_config.yaml \
--vllm-runtime-image=quay.io/modh/vllm@sha256:47b7e95b51d8d6a82de9d6bbc5eb61a794dc8d2998e8186bd6bd713517c97f08 \
--supported-accelerator-type=Nvidia \
--registry-host=registry.redhat.io \
  --snapshot-update \
  --log-file=pytest-logs.txt

or via CLI override:

uv run pytest -vv tests/model_serving/model_runtime/model_validation/test_modelvalidation.py \
  --supported-accelerator-type=Nvidia \
  --vllm-runtime-image=quay.io/modh/vllm@sha256:47b7e95b51d8d6a82de9d6bbc5eb61a794dc8d2998e8186bd6bd713517c97f08 \
  --registry-host=registry.redhat.io \
  --serving-argument='["--uvicorn-log-level=debug","--max-model-len=1024","--trust-remote-code"]' \
  --snapshot-update \
  --log-file=pytest-logs.txt

YAML

model-car:
  - name: granite-3.1-8b-base-quantized.w4a16
    image: oci://registry.redhat.io/rhelai1/modelcar-granite-3-1-8b-base-quantized-w4a16:1.5
  - name: Llama-3.1-8B-Instruct
    image: oci://registry.redhat.io/rhelai1/modelcar-llama-3-1-8b-instruct:1.5
  - name: Mistral-7B-Instruct-v0.3-quantized.w4a16
    image: oci://registry.redhat.io/rhelai1/modelcar-mistral-7b-instruct-v0-3-quantized-w4a16:1.5

serving_arguments:
  - "--uvicorn-log-level=debug"
  - "--max-model-len=1024"
  - "--trust-remote-code"
  - "--distributed-executor-backend=mp"

Validation checks included:

  • Confirmed OCI image resolution, secret injection, and serving runtime behavior.
  • Verified test output consistency across modelcar images.
--------------------------------- snapshot report summary ----------------------------------
12 snapshots generated.
======================= 6 passed in 1197.94s (0:19:57) =======================

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

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jun 8, 2025

📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • Added comprehensive model validation automation for vLLM modelcar deployments, including fixtures and utilities for flexible configuration and validation of inference services in both serverless and raw deployment modes.
    • Introduced support for loading model configurations and serving arguments from YAML files or CLI options.
  • Bug Fixes

    • Improved test skipping logic for deployment types to ensure accurate test execution conditions.
    • Enhanced error handling and validation in test fixtures for registry credentials and configuration loading.
  • Tests

    • Added new test suites and snapshot files to validate model serving and runtime behavior across multiple models and deployment types.
    • Updated and refactored test parameter names and fixtures for clarity and correctness.
  • Chores

    • Updated and added utility modules for model runtime and validation testing.
    • Refined snapshot data for ONNX model inference outputs.
## Summary by CodeRabbit

* **New Features**
  * Added comprehensive validation tests for vLLM model serving in both raw and serverless deployment modes, including OpenAI-style inference and snapshot-based output validation.
  * Introduced flexible test configuration via YAML files and command-line options for specifying model images, runtime arguments, and accelerator types.
  * Enhanced test fixtures for improved error handling, dynamic skipping, and support for multiple deployment scenarios.
  * Added utilities for Kubernetes secret management and safe resource naming in test environments.

* **Bug Fixes**
  * Corrected typographical errors in test parameter names to ensure consistent test execution.
  * Improved error handling in test fixtures to skip tests gracefully when required configuration is missing.

* **Tests**
  * Expanded test coverage with new snapshot files and validation logic for various model and deployment combinations.
  * Added utilities to validate inference outputs and facilitate raw and serverless inference requests.

* **Chores**
  * Updated and reorganized test fixtures and utility modules for maintainability and clarity.
  * Removed obsolete or redundant fixtures and imports to streamline the test suite.

## Walkthrough

This change introduces a new model validation automation framework for vLLM modelcar deployments, adding pytest configuration options, fixtures, test utilities, and a comprehensive test suite with snapshot validation. It enhances error handling, supports configuration via YAML, and removes the dependent operator verification fixture from model server tests. Numerous snapshot files and configuration examples are included.

## Changes

| Files / Paths                                                                                         | Change Summary                                                                                                               |
|------------------------------------------------------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------|
| conftest.py, tests/conftest.py                                                                       | Add new pytest CLI options and option groups for model validation; add/modify fixtures for registry, modelcar config, etc.   |
| tests/model_serving/conftest.py                                                                      | Update fixture to allow `models_s3_bucket_name` to be None, returning None for missing values.                              |
| tests/model_serving/model_runtime/conftest.py                                                        | Add fixtures for vLLM deployment skipping, pod retrieval, and deployment mode-based test skipping.                          |
| tests/model_serving/model_runtime/model_validation/conftest.py, constant.py, utils.py                | Introduce model validation test infra: fixtures for dynamic ISVC/runtime creation, config loading, snapshotting, utils.     |
| tests/model_serving/model_runtime/model_validation/sample_modelcar_config.yaml                        | Add example YAML config for modelcar validation (models, arguments, runtime image, accelerator, registry host).             |
| tests/model_serving/model_runtime/model_validation/test_modelvalidation.py                            | Add new test classes for validating vLLM modelcar serving in raw and serverless modes with OpenAI endpoint.                 |
| tests/model_serving/model_runtime/model_validation/__snapshots__/...                                 | Add multiple snapshot JSON files for model outputs and metadata for regression testing of modelcar validation.               |
| tests/model_serving/model_runtime/utils.py                                                           | Add utilities for inference validation, raw inference, OpenAI/TGIS endpoint handling, and deployment mode skipping.         |
| tests/model_serving/model_runtime/vllm/conftest.py                                                   | Fix typo in skip message, update return annotation, remove redundant deployment skipping fixtures.                          |
| tests/model_serving/model_server/conftest.py                                                         | Remove `fail_if_missing_dependent_operators` fixture and related imports.                                                   |
| tests/model_serving/model_runtime/vllm/basic_model_deployment/test_elyza_japanese_llama_2_7b_instruct.py<br>tests/model_serving/model_runtime/vllm/basic_model_deployment/test_granite_7b_redhat_lab.py<br>tests/model_serving/model_runtime/vllm/basic_model_deployment/test_granite_7b_starter.py<br>tests/model_serving/model_runtime/vllm/basic_model_deployment/test_llama31_8B_instruct.py<br>tests/model_serving/model_runtime/vllm/basic_model_deployment/test_llama3_8B_instruct.py<br>tests/model_serving/model_runtime/vllm/basic_model_deployment/test_merlinite_7b_lab.py | Fix typographical errors in fixture parameter names for deployment skipping in test methods.                                 |

## Possibly related PRs

- opendatahub-io/opendatahub-tests#297: Both PRs modify the `fail_if_missing_dependent_operators` fixture in `tests/model_serving/model_server/conftest.py`, with this PR removing it entirely and the related PR modifying its skip logic.

## Suggested labels

`size/l`, `Utilities`

## Suggested reviewers

- dbasunag
- mwaykole
- rhods-ci-bot

📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 94ddb39 and 9d9415b.

📒 Files selected for processing (1)
  • conftest.py (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • conftest.py
✨ Finishing Touches
  • 📝 Generate Docstrings

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
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 8, 2025

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

{'/hold', '/wip', '/verified', '/lgtm'}

@github-actions github-actions Bot added size/xxl and removed size/xl labels Jun 27, 2025
@github-actions github-actions Bot added size/xl and removed size/xxl labels Jun 30, 2025
@github-actions github-actions Bot added size/xxl and removed size/xl labels Jun 30, 2025
@edwardquarm edwardquarm marked this pull request as ready for review June 30, 2025 18:56
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
tests/model_serving/model_runtime/model_validation/sample_modelcar_config.yaml (1)

1-1: Prefer snake_case for root keys to match existing YAML style

Other config files in this repo use snake_case (e.g., mlserver_runtime, test_categories). Renaming model-carmodel_cars keeps style consistent and avoids the rare YAML tooling that treats hyphens as subtraction when templating.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5d796c2 and 97b8199.

📒 Files selected for processing (1)
  • tests/model_serving/model_runtime/model_validation/sample_modelcar_config.yaml (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#444
File: tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_mlserver_basic_model_deployment.py:48-714
Timestamp: 2025-07-16T12:20:29.672Z
Learning: In tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_mlserver_basic_model_deployment.py, the same get_deployment_config_dict() function is called twice in each pytest.param because different fixtures (mlserver_inference_service and mlserver_serving_runtime) need the same deployment configuration data. This duplication is intentional to provide identical configuration to multiple fixtures.
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:733-770
Timestamp: 2025-07-17T15:42:23.880Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 and model_registry_instance_2 fixtures do not need explicit database dependency fixtures (like db_deployment_1, db_secret_1, etc.) in their function signatures. Pytest's dependency injection automatically handles the fixture dependencies when they reference db_name_1 and db_name_2 parameters. This is the correct pattern for these Model Registry instance fixtures.
Learnt from: brettmthompson
PR: opendatahub-io/opendatahub-tests#269
File: tests/model_serving/model_server/serverless/test_scale_to_zero.py:0-0
Timestamp: 2025-04-29T00:49:40.918Z
Learning: In test cases for the opendatahub-tests repository, when verifying specific behaviors resulting from a sequence of operations, hard-coded validation values like "configurationGeneration=3" can be appropriate assertions to validate expected states, especially when they represent an invariant that should remain consistent after specific operations.
🔇 Additional comments (1)
tests/model_serving/model_runtime/model_validation/sample_modelcar_config.yaml (1)

9-13: Re-evaluate use of --trust-remote-code; confirm security posture

--trust-remote-code disables safety rails and will execute arbitrary code from the model repository at import time.
If this flag is not strictly required for these three models, drop it; otherwise document the justification and ensure the test cluster runs in a hardened sandbox.

lugi0
lugi0 previously approved these changes Jul 18, 2025
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🔭 Outside diff range comments (1)
conftest.py (1)

52-149: Fix unused variable issue.

The modelcar_group variable is assigned but never used, which creates a linting error. Either remove it or use it to add options.

Apply this diff to fix the unused variable:

-    modelcar_group = parser.getgroup(name="Modelcar options")
+    # modelcar_group = parser.getgroup(name="Modelcar options")

Or if modelcar options are needed in the future, use the group to add options.

🧹 Nitpick comments (1)
tests/model_serving/model_runtime/model_validation/conftest.py (1)

161-190: Consider consolidating similar functions.

The functions work correctly but have similar logic that could be consolidated into a single parameterized function to reduce duplication.

Consider refactoring into a single function:

def build_deployment_params(name: str, image: str, deployment_type: str) -> tuple[Any, str]:
    test_id = f"{name}-{deployment_type.lower()}"
    mark = pytest.mark.rawdeployment if deployment_type == KServeDeploymentType.RAW_DEPLOYMENT else pytest.mark.serverless
    param = pytest.param(
        {"name": f"{deployment_type.lower()}-model-validation"},
        {"deployment_type": deployment_type},
        {
            "model_name": name,
            "model_car_image_uri": image,
        },
        {"deployment_type": deployment_type},
        id=test_id,
        marks=[mark],
    )
    return param, test_id

However, the current implementation is clear and maintainable as-is.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3735161 and 94ddb39.

📒 Files selected for processing (8)
  • conftest.py (5 hunks)
  • tests/conftest.py (3 hunks)
  • tests/model_serving/model_runtime/conftest.py (1 hunks)
  • tests/model_serving/model_runtime/model_validation/conftest.py (1 hunks)
  • tests/model_serving/model_runtime/model_validation/test_modelvalidation.py (1 hunks)
  • tests/model_serving/model_runtime/utils.py (1 hunks)
  • tests/model_serving/model_runtime/vllm/conftest.py (3 hunks)
  • tests/model_serving/model_runtime/vllm/utils.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
  • tests/model_serving/model_runtime/model_validation/test_modelvalidation.py
  • tests/model_serving/model_runtime/utils.py
  • tests/model_serving/model_runtime/vllm/conftest.py
  • tests/model_serving/model_runtime/conftest.py
  • tests/conftest.py
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Learnt from: Snomaan6846
PR: opendatahub-io/opendatahub-tests#444
File: tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_mlserver_basic_model_deployment.py:48-714
Timestamp: 2025-07-16T12:20:29.672Z
Learning: In tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_mlserver_basic_model_deployment.py, the same get_deployment_config_dict() function is called twice in each pytest.param because different fixtures (mlserver_inference_service and mlserver_serving_runtime) need the same deployment configuration data. This duplication is intentional to provide identical configuration to multiple fixtures.
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:733-770
Timestamp: 2025-07-17T15:42:23.880Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 and model_registry_instance_2 fixtures do not need explicit database dependency fixtures (like db_deployment_1, db_secret_1, etc.) in their function signatures. Pytest's dependency injection automatically handles the fixture dependencies when they reference db_name_1 and db_name_2 parameters. This is the correct pattern for these Model Registry instance fixtures.
Learnt from: brettmthompson
PR: opendatahub-io/opendatahub-tests#269
File: tests/model_serving/model_server/serverless/test_scale_to_zero.py:0-0
Timestamp: 2025-04-29T00:49:40.918Z
Learning: In test cases for the opendatahub-tests repository, when verifying specific behaviors resulting from a sequence of operations, hard-coded validation values like "configurationGeneration=3" can be appropriate assertions to validate expected states, especially when they represent an invariant that should remain consistent after specific operations.
tests/model_serving/model_runtime/vllm/utils.py (2)

Learnt from: Snomaan6846
PR: #444
File: tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_mlserver_basic_model_deployment.py:48-714
Timestamp: 2025-07-16T12:20:29.672Z
Learning: In tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_mlserver_basic_model_deployment.py, the same get_deployment_config_dict() function is called twice in each pytest.param because different fixtures (mlserver_inference_service and mlserver_serving_runtime) need the same deployment configuration data. This duplication is intentional to provide identical configuration to multiple fixtures.

Learnt from: Snomaan6846
PR: #328
File: tests/model_serving/model_runtime/mlserver/conftest.py:182-189
Timestamp: 2025-06-16T05:39:15.070Z
Learning: The create_isvc function in utilities/inference_utils.py uses the parameter name volumes_mounts (with underscore), not volume_mounts, for passing volume mount configurations to InferenceService resources.

conftest.py (7)

Learnt from: lugi0
PR: #446
File: tests/model_registry/conftest.py:733-770
Timestamp: 2025-07-17T15:42:23.880Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 and model_registry_instance_2 fixtures do not need explicit database dependency fixtures (like db_deployment_1, db_secret_1, etc.) in their function signatures. Pytest's dependency injection automatically handles the fixture dependencies when they reference db_name_1 and db_name_2 parameters. This is the correct pattern for these Model Registry instance fixtures.

Learnt from: lugi0
PR: #446
File: tests/model_registry/conftest.py:595-612
Timestamp: 2025-07-17T15:42:04.167Z
Learning: In tests/model_registry/conftest.py, the db_deployment_1 fixture (and similar duplicated resource fixtures) do not require the admin_client parameter or explicit dependencies on related fixtures like db_secret_1, db_pvc_1, and db_service_1, even though the original model_registry_db_deployment fixture includes these parameters.

Learnt from: adolfo-ab
PR: #334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.

Learnt from: dbasunag
PR: #354
File: tests/model_registry/rbac/conftest.py:166-175
Timestamp: 2025-06-16T11:25:39.599Z
Learning: In tests/model_registry/rbac/conftest.py, predictable names are intentionally used for test resources (like RoleBindings and groups) instead of random names. This design choice prioritizes exposing cleanup failures from previous test runs through name collisions rather than masking such issues with random names. The philosophy is that test failures should be observable and informative to help debug underlying infrastructure or cleanup issues.

Learnt from: dbasunag
PR: #401
File: tests/model_registry/rest_api/mariadb/conftest.py:89-110
Timestamp: 2025-07-04T00:17:47.799Z
Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.

Learnt from: lugi0
PR: #446
File: tests/model_registry/conftest.py:0-0
Timestamp: 2025-07-17T15:42:26.275Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 fixture (and similar duplicated Model Registry instance fixtures) do not require admin_client, db_deployment_1, or db_secret_1 parameters as explicit dependencies, even though these dependencies exist implicitly through the fixture dependency chain.

Learnt from: dbasunag
PR: #359
File: tests/model_registry/rbac/conftest.py:214-229
Timestamp: 2025-06-17T21:36:46.461Z
Learning: In ocp_resources library, when reading from existing OAuth resources (like OAuth(name="cluster")), the client parameter is not required. The resource can access .instance.spec to read configuration from the live cluster without needing to pass a client parameter.

tests/model_serving/model_runtime/model_validation/conftest.py (12)

Learnt from: Snomaan6846
PR: #444
File: tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_mlserver_basic_model_deployment.py:48-714
Timestamp: 2025-07-16T12:20:29.672Z
Learning: In tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_mlserver_basic_model_deployment.py, the same get_deployment_config_dict() function is called twice in each pytest.param because different fixtures (mlserver_inference_service and mlserver_serving_runtime) need the same deployment configuration data. This duplication is intentional to provide identical configuration to multiple fixtures.

Learnt from: lugi0
PR: #446
File: tests/model_registry/conftest.py:733-770
Timestamp: 2025-07-17T15:42:23.880Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 and model_registry_instance_2 fixtures do not need explicit database dependency fixtures (like db_deployment_1, db_secret_1, etc.) in their function signatures. Pytest's dependency injection automatically handles the fixture dependencies when they reference db_name_1 and db_name_2 parameters. This is the correct pattern for these Model Registry instance fixtures.

Learnt from: adolfo-ab
PR: #334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.

Learnt from: lugi0
PR: #446
File: tests/model_registry/conftest.py:595-612
Timestamp: 2025-07-17T15:42:04.167Z
Learning: In tests/model_registry/conftest.py, the db_deployment_1 fixture (and similar duplicated resource fixtures) do not require the admin_client parameter or explicit dependencies on related fixtures like db_secret_1, db_pvc_1, and db_service_1, even though the original model_registry_db_deployment fixture includes these parameters.

Learnt from: israel-hdez
PR: #346
File: tests/model_serving/model_server/inference_graph/conftest.py:85-92
Timestamp: 2025-06-11T16:40:11.593Z
Learning: The helper create_isvc (used in tests/model_serving utilities) already waits until the created InferenceService reports Condition READY=True before returning, so additional readiness waits in fixtures are unnecessary.

Learnt from: lugi0
PR: #446
File: tests/model_registry/conftest.py:0-0
Timestamp: 2025-07-17T15:42:26.275Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 fixture (and similar duplicated Model Registry instance fixtures) do not require admin_client, db_deployment_1, or db_secret_1 parameters as explicit dependencies, even though these dependencies exist implicitly through the fixture dependency chain.

Learnt from: dbasunag
PR: #338
File: tests/model_registry/rbac/test_mr_rbac.py:24-53
Timestamp: 2025-06-06T12:22:57.057Z
Learning: In the opendatahub-tests repository, prefer keeping test parameterization configurations inline rather than extracting them to separate variables/constants, as it makes triaging easier by avoiding the need to jump between different parts of the file to understand the test setup.

Learnt from: dbasunag
PR: #401
File: tests/model_registry/rest_api/mariadb/conftest.py:89-110
Timestamp: 2025-07-04T00:17:47.799Z
Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.

Learnt from: lugi0
PR: #446
File: tests/model_registry/conftest.py:579-591
Timestamp: 2025-07-17T15:43:04.876Z
Learning: In tests/model_registry/conftest.py, the db_service_1 and db_service_2 fixtures do not require the admin_client parameter for Service resource creation, despite the existing model_registry_db_service fixture using client=admin_client. This inconsistency was confirmed as intentional by user lugi0.

Learnt from: dbasunag
PR: #354
File: tests/model_registry/rbac/conftest.py:166-175
Timestamp: 2025-06-16T11:25:39.599Z
Learning: In tests/model_registry/rbac/conftest.py, predictable names are intentionally used for test resources (like RoleBindings and groups) instead of random names. This design choice prioritizes exposing cleanup failures from previous test runs through name collisions rather than masking such issues with random names. The philosophy is that test failures should be observable and informative to help debug underlying infrastructure or cleanup issues.

Learnt from: lugi0
PR: #446
File: tests/model_registry/conftest.py:579-591
Timestamp: 2025-07-17T15:43:04.876Z
Learning: In tests/model_registry/conftest.py, Service resources can be created without explicitly passing the admin_client parameter when using the context manager approach with "with Service()". The client parameter is optional for Service resource creation.

Learnt from: lugi0
PR: #446
File: tests/model_registry/conftest.py:666-676
Timestamp: 2025-07-17T15:41:54.284Z
Learning: In tests/model_registry/conftest.py, the db_secret_1 and db_secret_2 fixtures do not require the admin_client parameter in their signatures, unlike some other Secret fixtures in the codebase. The user lugi0 confirmed this is the correct pattern for these specific fixtures.

🧬 Code Graph Analysis (1)
conftest.py (1)
tests/conftest.py (1)
  • admin_client (66-67)
🪛 Ruff (0.12.2)
conftest.py

52-52: Local variable modelcar_group is assigned to but never used

Remove assignment to unused variable modelcar_group

(F841)

🪛 Flake8 (7.2.0)
conftest.py

[error] 52-52: local variable 'modelcar_group' is assigned to but never used

(F841)

🔇 Additional comments (10)
tests/model_serving/model_runtime/vllm/utils.py (1)

219-221: LGTM! Function logic is correctly inverted.

The function properly implements the inverted skip logic by checking if the deployment mode does NOT match the specified type and provides a clear, informative skip message.

conftest.py (2)

39-39: LGTM! Import is necessary and properly used.

The ClusterServiceVersion import is required for the new operator verification fixture.


447-478: LGTM! Well-implemented operator verification fixture.

The fixture properly centralizes operator dependency verification by:

  • Checking ClusterServiceVersions in the configured namespace
  • Verifying operator readiness status
  • Providing clear failure messages for missing or non-ready operators

This is a good replacement for the previously removed fixture and follows pytest best practices.

tests/model_serving/model_runtime/model_validation/conftest.py (7)

1-37: LGTM! Well-organized imports.

The imports are properly structured with clear groupings for different functionality areas (pytest, Kubernetes resources, utilities, constants).


39-59: LGTM! Well-implemented serving runtime fixture.

The fixture properly:

  • Selects appropriate templates based on accelerator type
  • Uses context manager for automatic resource cleanup
  • Follows pytest fixture best practices with class scope

62-114: LGTM! Comprehensive inference service fixture.

The fixture handles complex configurations well:

  • Proper GPU resource allocation and volume mounting for multi-GPU setups
  • Runtime argument processing with quantization validation
  • Kubernetes resource management with pull secrets and timeouts
  • Clean resource lifecycle management with context manager

117-136: LGTM! Proper registry secret implementation.

The fixture correctly creates a Docker config JSON secret with:

  • Proper authentication structure for the registry host
  • Additional metadata fields for access type and OCI host
  • Clean resource management with context manager

139-158: LGTM! Clean configuration fixture.

The fixture properly:

  • Selects appropriate base configuration based on deployment type
  • Merges serving arguments and sets sensible defaults
  • Provides a clean interface for deployment configuration

193-242: LGTM! Robust test generation implementation.

The function handles dynamic test parametrization well:

  • Proper YAML loading with validation
  • Good error handling for malformed data
  • Class-specific parameter generation logic
  • Clean pytest parametrization setup

245-247: LGTM! Simple and effective pod retrieval.

The fixture correctly retrieves the pod resource associated with the inference service using the existing utility function.

lugi0
lugi0 previously approved these changes Jul 22, 2025
@mwaykole
Copy link
Copy Markdown
Member

hey can u please try running smoke justv to make sure nothing is breaking ?

@edwardquarm
Copy link
Copy Markdown
Contributor Author

hey can u please try running smoke justv to make sure nothing is breaking ?

I just rerun and everything looks good

Test results for 10 Raw and 10 Serverless deployments - 20 deployments in total
======================= 19 passed, 3854 warnings, 1 error in 6744.28s (1:52:24) =======================

error not related to code base issues

@edwardquarm edwardquarm changed the title model validation automation initial commit model validation automation v1 Jul 22, 2025
@Raghul-M Raghul-M enabled auto-merge (squash) July 23, 2025 06:28
@Raghul-M Raghul-M merged commit e91a879 into opendatahub-io:main Jul 23, 2025
10 checks passed
@github-actions
Copy link
Copy Markdown

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

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.

6 participants