Skip to content

Removing granite embedding dependency temporaily for EC fix.#2969

Open
sriroopar wants to merge 1 commit into
openshift:mainfrom
sriroopar:ec_handle_granite_embedding
Open

Removing granite embedding dependency temporaily for EC fix.#2969
sriroopar wants to merge 1 commit into
openshift:mainfrom
sriroopar:ec_handle_granite_embedding

Conversation

@sriroopar

@sriroopar sriroopar commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Description

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change

Related Tickets & Documents

  • Related Issue #
  • Closes #

Checklist before requesting a review

  • I have performed a self-review of my code.
  • PR has passed all pre-merge test jobs.
  • If it is a core feature, I have added thorough tests.

Testing

  • Please provide detailed steps to perform tests related to this code change.
  • How were the fix/results from this change verified? Please provide relevant screenshots or results.

Summary by CodeRabbit

  • Changes
    • Streamlined embedding model packaging and runtime provisioning to include only sentence-transformers/all-mpnet-base-v2.
    • Disabled the previously bundled granite-embedding-30m-english model in the default artifact list and embedding setup.
    • Updated preload/cache preparation to match the single-model configuration.
    • Removed the dedicated embedding download step for the OKP/Solr binary; the aggregate embedding fetch target may no longer succeed as before.

@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@sriroopar, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 24 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 320414f7-f05d-4a41-8915-66a1eeb2ef18

📥 Commits

Reviewing files that changed from the base of the PR and between a2fd43c and 8495520.

📒 Files selected for processing (4)
  • Containerfile
  • Makefile
  • artifacts.lock.yaml
  • ols/constants.py
📝 Walkthrough

Walkthrough

The PR disables Granite embedding model handling. It comments out the Granite artifact and constant, removes the OKP embedding Makefile target, and narrows the container runtime to all-mpnet-base-v2 only.

Changes

Disable Granite embedding model path

Layer / File(s) Summary
Artifact and constant removal
artifacts.lock.yaml, ols/constants.py
The Granite embedding artifact entry and EMBEDDINGS_MODEL_OKP_SUBDIR constant are commented out.
Container runtime narrowed
Containerfile
Runtime copy, download, and HuggingFace cache pre-population are limited to all-mpnet-base-v2; Granite handling is removed and marked temporarily disabled for EC compliance.
Embedding target removed
Makefile
The get-embeddings-okp target and its download recipe are deleted.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title matches the main change: temporarily removing the granite embedding dependency to address the EC fix.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@openshift-ci openshift-ci Bot requested review from blublinsky and raptorsun June 26, 2026 14:10
@raptorsun

Copy link
Copy Markdown
Contributor

/approve
/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2026
@openshift-ci

openshift-ci Bot commented Jun 26, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: raptorsun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 26, 2026
@sriroopar sriroopar force-pushed the ec_handle_granite_embedding branch from 231fdee to d921e4d Compare June 29, 2026 13:55
@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 29, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
ols/constants.py (1)

33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove the commented-out OKP constant.

Keeping the granite path here leaves a stale embedding reference in the config module even though the repo now provisions only all-mpnet-base-v2, and the comment itself violates the repo's no-comments rule.

Suggested cleanup
 EMBEDDINGS_MODEL_BYOK_SUBDIR = "all-mpnet-base-v2"
-# EMBEDDINGS_MODEL_OKP_SUBDIR = "granite-embedding-30m-english"

As per coding guidelines, "Code should be self-documenting; no comments unless explicitly requested."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ols/constants.py` at line 33, Remove the commented-out
EMBEDDINGS_MODEL_OKP_SUBDIR constant from constants.py; it is stale, references
the old granite embedding path, and violates the no-comments guideline. Clean up
the constants module so only the active embeddings configuration remains, and
verify any references rely on the current all-mpnet-base-v2 setup rather than
the removed OKP symbol.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@ols/constants.py`:
- Line 33: Remove the commented-out EMBEDDINGS_MODEL_OKP_SUBDIR constant from
constants.py; it is stale, references the old granite embedding path, and
violates the no-comments guideline. Clean up the constants module so only the
active embeddings configuration remains, and verify any references rely on the
current all-mpnet-base-v2 setup rather than the removed OKP symbol.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 12fdb111-6cfd-4c50-a128-24c84f0e0840

📥 Commits

Reviewing files that changed from the base of the PR and between 272684d and d921e4d.

📒 Files selected for processing (11)
  • Containerfile
  • Makefile
  • artifacts.lock.yaml
  • embeddings_model/granite-embedding-30m-english/1_Pooling/config.json
  • embeddings_model/granite-embedding-30m-english/config.json
  • embeddings_model/granite-embedding-30m-english/modules.json
  • embeddings_model/granite-embedding-30m-english/sentence_bert_config.json
  • embeddings_model/granite-embedding-30m-english/special_tokens_map.json
  • embeddings_model/granite-embedding-30m-english/tokenizer.json
  • embeddings_model/granite-embedding-30m-english/tokenizer_config.json
  • ols/constants.py
💤 Files with no reviewable changes (7)
  • embeddings_model/granite-embedding-30m-english/sentence_bert_config.json
  • embeddings_model/granite-embedding-30m-english/special_tokens_map.json
  • embeddings_model/granite-embedding-30m-english/config.json
  • embeddings_model/granite-embedding-30m-english/modules.json
  • embeddings_model/granite-embedding-30m-english/1_Pooling/config.json
  • embeddings_model/granite-embedding-30m-english/tokenizer_config.json
  • Makefile
🚧 Files skipped from review as they are similar to previous changes (2)
  • artifacts.lock.yaml
  • Containerfile

@sriroopar sriroopar force-pushed the ec_handle_granite_embedding branch from d921e4d to 083910c Compare June 29, 2026 13:58
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 29, 2026
@sriroopar

Copy link
Copy Markdown
Contributor Author

/retest

1 similar comment
@sriroopar

Copy link
Copy Markdown
Contributor Author

/retest

@blublinsky

Copy link
Copy Markdown
Contributor

/retest
/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jun 29, 2026
@sriroopar sriroopar force-pushed the ec_handle_granite_embedding branch from 6ed1b34 to cf3853e Compare June 29, 2026 18:45
@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 29, 2026
@openshift-ci

openshift-ci Bot commented Jun 29, 2026

Copy link
Copy Markdown

New changes are detected. LGTM label has been removed.

@openshift-ci openshift-ci Bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 29, 2026
@sriroopar sriroopar force-pushed the ec_handle_granite_embedding branch from 7e991a1 to ce843fb Compare June 29, 2026 20:02
@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 29, 2026
Comment out granite-embedding-30m-english references to avoid
disallowed source EC violation. The model files are removed and
URL references are commented out for future re-enablement.

Co-authored-by: Cursor <cursoragent@cursor.com>
@sriroopar sriroopar force-pushed the ec_handle_granite_embedding branch from ce843fb to 8495520 Compare June 29, 2026 20:18
@sriroopar sriroopar force-pushed the ec_handle_granite_embedding branch from a332981 to a2b75fc Compare June 29, 2026 20:24
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 29, 2026
@sriroopar sriroopar force-pushed the ec_handle_granite_embedding branch 3 times, most recently from 1d7e243 to 8495520 Compare June 29, 2026 21:46
@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 29, 2026
@openshift-ci

openshift-ci Bot commented Jun 29, 2026

Copy link
Copy Markdown

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-ci

openshift-ci Bot commented Jun 29, 2026

Copy link
Copy Markdown

@sriroopar: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/integration 8495520 link true /test integration
ci/prow/unit 8495520 link true /test unit
ci/prow/e2e-ols-cluster 8495520 link true /test e2e-ols-cluster
ci/prow/security 8495520 link false /test security
ci/prow/images 8495520 link true /test images
ci/prow/fips-image-scan-service 8495520 link true /test fips-image-scan-service
ci/prow/ols-evaluation 8495520 link true /test ols-evaluation
ci/prow/verify 8495520 link true /test verify

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants