Skip to content

Conversation

@are-ces
Copy link
Contributor

@are-ces are-ces commented Oct 29, 2025

Description

Parametrized and updated the model used for RHAIIS, changed the shield model to use openai

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up service version
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change
  • Unit tests improvement
  • Integration tests improvement
  • End to end tests improvement

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

  • New Features

    • Added support for GPT-4-turbo model option.
    • Integrated OpenAI as a remote inference provider.
  • Chores

    • Updated environment and configuration settings to support flexible model selection and provider options.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 29, 2025

Walkthrough

This change configures e2e testing infrastructure to support OpenAI models alongside existing vLLM deployments. Environment variables for OpenAI API key and model ID are added to the GitHub Actions workflow and Docker Compose setup. The test configuration is updated to include an OpenAI provider, new gpt-4-turbo model entry, and shield configuration pointing to gpt-4-turbo.

Changes

Cohort / File(s) Summary
Test Infrastructure
.github/workflows/e2e_tests_rhaiis.yaml, docker-compose.yaml
Added OPENAI_API_KEY (from secrets) and RHAIIS_MODEL (set to meta-llama/Llama-3.1-8B) environment variables to workflow and Docker Compose service; added LLAMA_STACK_LOGGING=all=debug to Docker Compose.
Test Configuration
tests/e2e/configs/run-rhaiis.yaml
Added remote OpenAI provider with API key from environment; updated provider shield from "meta-llama/Llama-3.1-8B-Instruct" to "gpt-4-turbo"; replaced primary llm model to use env.RHAIIS_MODEL via vLLM provider; added new gpt-4-turbo model entry using OpenAI provider.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Configuration-only changes across three YAML files with no control-flow or logic modifications
  • Straightforward environment variable and model/provider additions following existing patterns
  • Primary review focus: verify environment variable naming consistency and that provider/model IDs align with external service expectations (OpenAI API, vLLM configuration)

Possibly related PRs

Suggested reviewers

  • tisnik
  • radofuchs

Poem

🐰 A rabbit hops through configs bright,
Adding shields of turbo might—
OpenAI joins the llama crew,
E2E tests now have what's new!
🔑✨

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 "RHAIIS e2e tests: Fix model change" is directly related to the changeset and accurately captures the main intent of the pull request. The changes across all three files involve parametrizing and reconfiguring the models used in RHAIIS e2e tests, including adding environment variables, updating the shield model to use OpenAI, and adjusting model configurations in the test setup. The title is concise, specific to the component (RHAIIS e2e tests), and clearly conveys that this is a fix addressing model configuration changes, which aligns well with the PR objectives and the actual modifications.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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
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)
docker-compose.yaml (1)

20-21: Consider making debug logging conditional in e2e tests.

The LLAMA_STACK_LOGGING=all=debug is now permanent for all e2e test runs. Debug logging can impact performance and increase verbosity. Consider gating this behind a conditional environment variable so it can be toggled on/off per test run.

Example:

- LLAMA_STACK_LOGGING=${LLAMA_STACK_LOGGING:-all=info}

This allows tests to run with default info level but enable debug when needed for diagnostics.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8aca35f and 6880c87.

📒 Files selected for processing (3)
  • .github/workflows/e2e_tests_rhaiis.yaml (1 hunks)
  • docker-compose.yaml (2 hunks)
  • tests/e2e/configs/run-rhaiis.yaml (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: build-pr
  • GitHub Check: e2e_tests (ci)
  • GitHub Check: e2e_tests (azure)
🔇 Additional comments (4)
.github/workflows/e2e_tests_rhaiis.yaml (1)

17-20: Verify GitHub secret exists and reconsider hardcoded model value.

Line 17 references secrets.OPENAI_API_KEY, which must pre-exist in your GitHub repository secrets. If missing, the workflow will fail. Verify this secret is configured.

Line 20 hardcodes RHAIIS_MODEL: meta-llama/Llama-3.1-8B, but the PR summary claims the model was "parametrized." If the intent is parametrization for flexibility, consider using a secret or input parameter instead of a hardcoded value. This would allow testing different models without modifying workflow code.

tests/e2e/configs/run-rhaiis.yaml (3)

82-85: LGTM: OpenAI provider configuration.

The new OpenAI provider is properly configured with environment-sourced API key. Structure and syntax are correct.


141-148: LGTM: Model parametrization and gpt-4-turbo configuration.

The main model is now correctly parametrized via env.RHAIIS_MODEL with vllm provider. The new gpt-4-turbo model entry properly links to the OpenAI provider. Configuration structure is sound.


130-133: Review concern about provider/model compatibility requires clarification of intended shield architecture.

The review identifies a pattern where provider_shield_id: gpt-4-turbo (OpenAI) is paired with provider_id: llama-guard across multiple configurations. However, investigation reveals:

Key findings:

  • Git history shows this was a deliberate change across all config files: meta-llama/Llama-3.1-8B-Instructgpt-4-turbo
  • The inline comment in run.yaml (line 137) says "# Model to use for safety checks" — phrasing suggests provider-agnostic resource selection
  • The shields endpoint performs no local validation of provider/model compatibility; it delegates to the external Llama Stack service
  • Codebase contains no provider compatibility constraints

Critical uncertainty:
Whether provider_shield_id is designed as provider-specific (your assumption) or provider-agnostic (the pattern suggests). The shields architecture does not enforce this constraint locally — the external Llama Stack service handles actual resolution.

To confirm if this is an actual compatibility issue or intentional design, verify:

  1. Llama Stack documentation: does llama-guard provider support arbitrary model identifiers?
  2. Whether the current configurations are actively tested and passing

Copy link
Contributor

@tisnik tisnik left a comment

Choose a reason for hiding this comment

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

LGTM

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants