Skip to content

Revert ModelCar change to Gaussian credit model#885

Merged
sheltoncyril merged 2 commits intoopendatahub-io:mainfrom
sheltoncyril:gaussian-credit-model-revert
Dec 1, 2025
Merged

Revert ModelCar change to Gaussian credit model#885
sheltoncyril merged 2 commits intoopendatahub-io:mainfrom
sheltoncyril:gaussian-credit-model-revert

Conversation

@sheltoncyril
Copy link
Copy Markdown
Contributor

@sheltoncyril sheltoncyril commented Nov 27, 2025

Just reverting this as it does not seem to work well with Seldon ML Server

Summary by CodeRabbit

  • Tests
    • Updated test infrastructure for model configuration and storage handling.

✏️ 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

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

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Nov 27, 2025

📝 Walkthrough

Walkthrough

The conftest.py test fixture file was updated to replace hardcoded string literals with imported constants for model naming and storage configuration. Storage parameter changed from storage_uri to storage_key and storage_path parameters using the imported constant.

Changes

Cohort / File(s) Summary
Test fixture constants refactoring
tests/model_explainability/trustyai_service/conftest.py
Added imports for GAUSSIAN_CREDIT_MODEL_STORAGE_PATH and GAUSSIAN_CREDIT_MODEL. Updated gaussian_credit_model fixture to use GAUSSIAN_CREDIT_MODEL constant instead of literal "models" for InferenceService name. Replaced storage_uri parameter with storage_key and storage_path parameters using GAUSSIAN_CREDIT_MODEL_STORAGE_PATH constant.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • The changes follow a consistent pattern of replacing hardcoded literals with imported constants
  • Single file modification with straightforward parameter renaming in a test fixture
  • No complex logic changes, primarily configuration-level updates

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly matches the PR's objective of reverting a ModelCar change related to the Gaussian credit model, accurately summarizing the main change.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


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.

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

🧹 Nitpick comments (1)
tests/model_explainability/trustyai_service/conftest.py (1)

290-294: gaussian_credit_model fixture changes look consistent with MinIO data-connection usage

Switching the ISVC name to GAUSSIAN_CREDIT_MODEL and wiring storage_key=minio_data_connection.name plus storage_path=GAUSSIAN_CREDIT_MODEL_STORAGE_PATH matches the pattern implied by the minio_data_connection fixture and keeps the storage details centralized in constants. Behavior should remain predictable across tests, and I don’t see any functional problems.

If you want to tighten readability, you could add a short comment clarifying that storage_key is the MinIO data-connection Secret name and storage_path is the model path within the bucket, but that’s optional.

Also applies to: 301-313

📜 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 4c8b808 and 8377a01.

📒 Files selected for processing (1)
  • tests/model_explainability/trustyai_service/conftest.py (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/model_explainability/trustyai_service/conftest.py (1)
tests/conftest.py (1)
  • minio_data_connection (590-602)
🔇 Additional comments (1)
tests/model_explainability/trustyai_service/conftest.py (1)

25-39: Using GAUSSIAN_CREDIT_MODEL constants in imports looks good

Pulling GAUSSIAN_CREDIT_MODEL and GAUSSIAN_CREDIT_MODEL_STORAGE_PATH from the shared constants module removes hard‑coded strings and keeps the fixture aligned with any future changes to naming or storage config. No issues here.

Copy link
Copy Markdown
Contributor

@kpunwatk kpunwatk 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

@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

@sheltoncyril sheltoncyril merged commit 3a0c2cf into opendatahub-io:main Dec 1, 2025
9 checks passed
@github-actions
Copy link
Copy Markdown

github-actions bot commented Dec 1, 2025

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

@sheltoncyril
Copy link
Copy Markdown
Contributor Author

/cherry-pick 2.25

@rhods-ci-bot
Copy link
Copy Markdown
Contributor

Cherry pick action created PR #909 successfully 🎉!
See: https://github.com/opendatahub-io/opendatahub-tests/actions/runs/19924624090

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.

5 participants