(feat): autorag migrate from chat.completions to responses#79
Open
filip-komarzyniec wants to merge 11 commits into
Open
Conversation
Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
…plicating ai4rag behaviour Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
…ve script; related changes Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR refactors AutoRAG component testing infrastructure and removes the deployment layer. It establishes pytest hooks for test collection with AI4RAG mocking, removes the build_responses_request_bodies component entirely, adds KFP install-control flags, parameterizes helper functions to pass S3 credentials and test data explicitly (eliminating closure dependencies), replaces OGX clients with Llama Stack clients using improved SSL-fallback logic, introduces AWK/bash tooling for automated nested-function extraction, generates comprehensive test suites for 1000+ lines of new test code, and updates project configuration. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/data_processing/autorag/documents_discovery/component.py (1)
75-83:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle optional region lookup defensively in
_make_s3_client.At Line 79,
s3_creds["AWS_DEFAULT_REGION"]can raiseKeyErrorwhen the optional region key is absent. Use.get(...)to keep this helper tolerant of region-less creds.Suggested patch
def _make_s3_client(s3_creds, verify=True): return boto3.client( "s3", endpoint_url=s3_creds["AWS_S3_ENDPOINT"], - region_name=s3_creds["AWS_DEFAULT_REGION"], + region_name=s3_creds.get("AWS_DEFAULT_REGION"), aws_access_key_id=s3_creds["AWS_ACCESS_KEY_ID"], aws_secret_access_key=s3_creds["AWS_SECRET_ACCESS_KEY"], verify=verify, )🤖 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 `@components/data_processing/autorag/documents_discovery/component.py` around lines 75 - 83, The _make_s3_client helper currently accesses s3_creds["AWS_DEFAULT_REGION"] which can raise KeyError for region-less credentials; change that access to use s3_creds.get("AWS_DEFAULT_REGION") (or similar defensive lookup) when calling boto3.client inside _make_s3_client so the region parameter is None when absent and the function remains tolerant of creds without AWS_DEFAULT_REGION while leaving other keys and the verify parameter intact.
🧹 Nitpick comments (1)
components/training/autorag/leaderboard_evaluation/tests/test_nested_names.py (1)
243-263: ⚡ Quick winStop codifying crash behavior for malformed pattern payloads.
These tests currently require
AttributeErroron malformed structures, which hard-locks non-resilient behavior. Update expectations to graceful fallback (None/(True, 0)), then harden the helper implementations accordingly.Also applies to: 662-671
🤖 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 `@components/training/autorag/leaderboard_evaluation/tests/test_nested_names.py` around lines 243 - 263, Update the tests to expect graceful fallbacks instead of AttributeError for malformed rag_pattern payloads: change assertions in test_non_dict_rag_pattern and test_non_dict_settings to assert that _settings_from_rag_pattern(e) returns None (or the documented default like (True, 0) where applicable) rather than raising; then harden the helper _settings_from_rag_pattern to first validate types (e.g., if not isinstance(e.get("rag_pattern"), dict): return None) and similarly ensure rag_pattern.get("settings") is a dict before using .get(), returning the appropriate default/fallback when structure is malformed; apply the same pattern to the other tests/helpers that handle rag_pattern malformed inputs mentioned in the comment.
🤖 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.
Inline comments:
In `@components/data_processing/autorag/conftest.py`:
- Around line 11-13: The current conftest global update of sys.modules via
_AI4RAG_MOCKS = make_ai4rag_modules(); sys.modules.update(_AI4RAG_MOCKS) can
overwrite real ai4rag modules—change this to a session-scoped pytest fixture
that (1) iterates over keys from make_ai4rag_modules() and only inserts mocks
for keys missing in sys.modules, recording which keys were injected, and (2) in
fixture teardown restores original modules by deleting injected keys (or
restoring saved originals) so real modules are never permanently replaced;
reference the _AI4RAG_MOCKS variable and make_ai4rag_modules() when implementing
the fixture and remove the global sys.modules.update call.
In
`@components/data_processing/autorag/documents_indexing/tests/test_nested_names.py`:
- Around line 10-13: The test imports reference helpers that don't exist and
validate different client names: update the code so tests and implementation
match by either extracting the helper functions/classes into the expected module
or updating test imports; specifically either (A) create
components.data_processing.autorag.documents_indexing.tests.nested_names with
the helper functions and predicates used by TestCreateLlamaStackClient
(exporting _create_llama_stack_client and _is_ssl_error) and wire it to
construct/return the existing implementation, or (B) change the test to import
the actual implementation symbols (replace _create_llama_stack_client and
LlamaStackClient references in TestCreateLlamaStackClient with
_create_ogx_client and OgxClient and adjust any helper name differences) so
imports and assertions match the implementation.
In `@components/data_processing/autorag/test_data_loader/component.py`:
- Around line 74-93: The SSLError retry path bypasses the ClientError handling
and also raises TestDataLoaderException with tuple-style args; update the
SSLError except block to wrap the verify=False download call in the same
try/except handling as the primary download (catch ClientError and map
404/NoSuchKey to FileNotFoundError, re-raise other ClientError and generic
Exception as TestDataLoaderException using a formatted string), i.e., call
_make_s3_client(..., verify=False) then call s3_client.download_file(...) inside
a try/except that mirrors the existing ClientError/Exception branches and use
formatted messages (f-strings or .format) when constructing
TestDataLoaderException so the message is a string and preserve "from e" when
re-raising; reference TestDataLoaderException, _make_s3_client,
s3_client.download_file, ClientError, SSLError, test_data_bucket_name and
test_data_path to locate changes.
In `@components/training/autorag/conftest.py`:
- Around line 11-13: The test setup currently unconditionally overwrites
sys.modules with _AI4RAG_MOCKS from make_ai4rag_modules(), which can clobber
real ai4rag modules; change this to only insert missing keys and record which
keys you injected so you can restore state on teardown: iterate over
_AI4RAG_MOCKS and for each module name check if it exists in sys.modules before
setting it, store a set/list of newly-inserted names, and in the pytest
teardown/fixture finalizer remove those injected keys (or restore original
values if you saved them) to avoid leaking mocks; reference make_ai4rag_modules,
_AI4RAG_MOCKS, and sys.modules when making the changes.
In `@components/training/autorag/rag_templates_optimization/component.py`:
- Around line 622-628: The template generation assumes
pattern_data["payload"]["settings"]["responses_template"] exists and opens a
relative template file, which can raise KeyError or fail if CWD differs; update
the code around template_context, model_responses_templ and the template file
open to: read responses_template using guarded access (e.g., use .get on
payload/settings with a sensible default or validate and raise a clear error if
missing), resolve the template path using
Path(__file__).parent.joinpath("create_model_response.templ.py") (or similar)
instead of a bare filename, and handle file I/O errors (FileNotFoundError) with
a clear log/exception so writing to (patt_dir / "create_model_response.py") only
proceeds when the template was successfully loaded and template_context
populated.
- Around line 104-109: The OGX server probe uses httpx.get without a timeout
which can hang; update both httpx.get calls in the try/except block (the initial
probe and the fallback call with verify=False) to include an explicit timeout
(e.g., timeout=10.0) so the probing in component.py (inside the autorag RAG
template logic) fails fast instead of blocking the training pipeline.
In
`@components/training/autorag/rag_templates_optimization/notebook_templates/ogx_indexing_template.ipynb`:
- Around line 317-343: The call to create_ogx_client at the bottom passes the
API key and base URL in the wrong order; update the client instantiation so the
first argument is the base URL and the second is the api key (use
os.getenv("OGX_CLIENT_BASE_URL") as the first param and
os.getenv("OGX_CLIENT_API_KEY") as the second) when calling create_ogx_client to
match the function signature and ensure OgxClient is initialized correctly.
In
`@components/training/autorag/rag_templates_optimization/notebook_templates/ogx_inference_template.ipynb`:
- Around line 106-132: The call that creates the client passes arguments to
create_ogx_client in the wrong order; swap the two environment variables so
create_ogx_client receives base_url first and api_key second. Locate the
invocation of create_ogx_client that assigns to client and change the argument
order so the first parameter is the OGX_CLIENT_BASE_URL value and the second is
the OGX_CLIENT_API_KEY value, leaving the create_ogx_client function body
unchanged.
In
`@components/training/autorag/rag_templates_optimization/tests/scripts/orchestrate_extraction.sh`:
- Around line 1-29: The script lacks strict mode and proper argument validation
and also uses unquoted positional parameters ($1, $2) which enables
word-splitting/globbing; add strict mode (set -euo pipefail) at the top,
validate the expected argument count (ensure two args) and emit a clear usage
error then exit non-zero if missing, and quote all shell variable expansions in
the awk invocation and echo messages (replace $1/$2 with "$1"/"$2"). Ensure you
still capture awk's exit code into rc and exit with that code after logging;
target the awk invocation line and the initial argument checks in this script
when making the changes.
In
`@components/training/autorag/rag_templates_optimization/tests/test_nested_names.py`:
- Around line 562-564: The test calls generate_notebook_from_templates with old
template names ("ls_inference"/"ls_indexing"); update the test to use the
component's current OGX names ("ogx_inference" and "ogx_indexing") so the
generation tests match runtime behavior. Locate the invocation of
generate_notebook_from_templates in test_nested_names.py and replace the
notebook_template and any index_template arguments that reference "ls_*" to
"ogx_*" (e.g., notebook_template="ogx_inference", index_template="ogx_indexing")
and run the test to ensure templates resolve correctly.
- Around line 470-479: Tests are asserting old parameters (chat_model_url,
embedding_model_url, ls_ssl_verify, chat_ssl_verify, embedding_ssl_verify) for
create_placeholder_mapping; update the test cases in
components/training/autorag/rag_templates_optimization/tests/test_nested_names.py
to match the current create_placeholder_mapping contract in component.py by
removing references and assertions for those deprecated parameters/keys and
instead assert the current expected keys/parameters returned by
create_placeholder_mapping ( locate usages in the file where
create_placeholder_mapping is called — e.g., the call assigning to result — and
update the assertions around result to reflect the new API ); apply the same
change to the other call sites in the file that mirror this pattern.
In
`@components/training/autorag/search_space_preparation/tests/test_component_unit.py`:
- Around line 350-359: The test is passing unexpected kwargs to
search_space_preparation.python_func (chat_model_url, chat_model_token,
embedding_model_url, embedding_model_token) which causes a TypeError before the
OGXAPIConnectionError path; fix the tests by removing those four kwargs from the
python_func invocation (or alternatively add corresponding parameters to
search_space_preparation.python_func if intentional), ensuring the call uses
only the function's accepted arguments so the code executes to the API
connection error path.
- Around line 365-366: The tests call undefined helper factories
(_make_openai_module and _make_llama_stack_client_module) which causes
NameError; either import these factory functions from the test utilities module
where they are defined or implement local factory stubs that return the expected
fake modules/clients used in the tests (e.g., return an object exposing
APIConnectionError for OpenAI and the expected attributes for LlamaStack);
update the top of the test module to add the imports or the small helper
implementations and run the tests to ensure OAIAPIConnectionError and the llama
client references resolve correctly.
In
`@components/training/autorag/search_space_preparation/tests/test_nested_names.py`:
- Around line 639-640: The test currently mutates a Mock by deleting
mock_params.model_dump to simulate Pydantic v1, which is fragile; instead
replace mock_params with a simple stub object that lacks the model_dump
attribute and implements dict() returning {"embedding_dimension": 512}. Locate
the test helper/fixture where mock_params is constructed (references:
mock_params, model_dump, dict.return_value) and create or instantiate a plain
class or SimpleNamespace with a dict() method (or attribute) returning the
expected dict, then use that stub in the assertions; do the same replacement for
the other occurrence around lines referencing the second deletion (the one at
742-743).
---
Outside diff comments:
In `@components/data_processing/autorag/documents_discovery/component.py`:
- Around line 75-83: The _make_s3_client helper currently accesses
s3_creds["AWS_DEFAULT_REGION"] which can raise KeyError for region-less
credentials; change that access to use s3_creds.get("AWS_DEFAULT_REGION") (or
similar defensive lookup) when calling boto3.client inside _make_s3_client so
the region parameter is None when absent and the function remains tolerant of
creds without AWS_DEFAULT_REGION while leaving other keys and the verify
parameter intact.
---
Nitpick comments:
In
`@components/training/autorag/leaderboard_evaluation/tests/test_nested_names.py`:
- Around line 243-263: Update the tests to expect graceful fallbacks instead of
AttributeError for malformed rag_pattern payloads: change assertions in
test_non_dict_rag_pattern and test_non_dict_settings to assert that
_settings_from_rag_pattern(e) returns None (or the documented default like
(True, 0) where applicable) rather than raising; then harden the helper
_settings_from_rag_pattern to first validate types (e.g., if not
isinstance(e.get("rag_pattern"), dict): return None) and similarly ensure
rag_pattern.get("settings") is a dict before using .get(), returning the
appropriate default/fallback when structure is malformed; apply the same pattern
to the other tests/helpers that handle rag_pattern malformed inputs mentioned in
the comment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 748e485a-001d-4a4e-a1ea-7adb3d3b54ff
📒 Files selected for processing (41)
components/data_processing/autorag/conftest.pycomponents/data_processing/autorag/documents_discovery/component.pycomponents/data_processing/autorag/documents_discovery/tests/test_component_unit.pycomponents/data_processing/autorag/documents_discovery/tests/test_nested_names.pycomponents/data_processing/autorag/documents_indexing/component.pycomponents/data_processing/autorag/documents_indexing/tests/test_component_unit.pycomponents/data_processing/autorag/documents_indexing/tests/test_nested_names.pycomponents/data_processing/autorag/test_data_loader/component.pycomponents/data_processing/autorag/tests/ai4rag_mocks.pycomponents/data_processing/autorag/text_extraction/component.pycomponents/data_processing/autorag/text_extraction/tests/test_component_unit.pycomponents/data_processing/autorag/text_extraction/tests/test_nested_names.pycomponents/deployment/autorag/OWNERScomponents/deployment/autorag/README.mdcomponents/deployment/autorag/__init__.pycomponents/deployment/autorag/build_responses_request_bodies/OWNERScomponents/deployment/autorag/build_responses_request_bodies/README.mdcomponents/deployment/autorag/build_responses_request_bodies/__init__.pycomponents/deployment/autorag/build_responses_request_bodies/component.pycomponents/deployment/autorag/build_responses_request_bodies/example_pipelines.pycomponents/deployment/autorag/build_responses_request_bodies/metadata.yamlcomponents/deployment/autorag/build_responses_request_bodies/tests/__init__.pycomponents/deployment/autorag/build_responses_request_bodies/tests/openai_responses_request_validate.pycomponents/deployment/autorag/build_responses_request_bodies/tests/test_component_unit.pycomponents/deployment/autorag/build_responses_request_bodies/tests/test_openai_responses_api_compliance.pycomponents/training/autorag/conftest.pycomponents/training/autorag/leaderboard_evaluation/component.pycomponents/training/autorag/leaderboard_evaluation/tests/test_nested_names.pycomponents/training/autorag/rag_templates_optimization/component.pycomponents/training/autorag/rag_templates_optimization/create_model_response.templ.pycomponents/training/autorag/rag_templates_optimization/notebook_templates/ogx_indexing_template.ipynbcomponents/training/autorag/rag_templates_optimization/notebook_templates/ogx_inference_template.ipynbcomponents/training/autorag/rag_templates_optimization/tests/conftest.pycomponents/training/autorag/rag_templates_optimization/tests/scripts/extract_nested_funcs.awkcomponents/training/autorag/rag_templates_optimization/tests/scripts/orchestrate_extraction.shcomponents/training/autorag/rag_templates_optimization/tests/test_component_unit.pycomponents/training/autorag/rag_templates_optimization/tests/test_nested_names.pycomponents/training/autorag/search_space_preparation/component.pycomponents/training/autorag/search_space_preparation/tests/test_component_unit.pycomponents/training/autorag/search_space_preparation/tests/test_nested_names.pycomponents/training/autorag/tests/ai4rag_mocks.py
💤 Files with no reviewable changes (17)
- components/deployment/autorag/build_responses_request_bodies/OWNERS
- components/deployment/autorag/build_responses_request_bodies/tests/openai_responses_request_validate.py
- components/deployment/autorag/build_responses_request_bodies/tests/init.py
- components/deployment/autorag/README.md
- components/deployment/autorag/OWNERS
- components/deployment/autorag/build_responses_request_bodies/README.md
- components/deployment/autorag/build_responses_request_bodies/example_pipelines.py
- components/deployment/autorag/build_responses_request_bodies/metadata.yaml
- components/deployment/autorag/build_responses_request_bodies/tests/test_openai_responses_api_compliance.py
- components/deployment/autorag/build_responses_request_bodies/tests/test_component_unit.py
- components/data_processing/autorag/documents_discovery/tests/test_component_unit.py
- components/deployment/autorag/build_responses_request_bodies/component.py
- components/deployment/autorag/init.py
- components/deployment/autorag/build_responses_request_bodies/init.py
- components/training/autorag/rag_templates_optimization/tests/test_component_unit.py
- components/data_processing/autorag/text_extraction/tests/test_component_unit.py
- components/data_processing/autorag/documents_indexing/tests/test_component_unit.py
Comment on lines
+74
to
+93
| except SSLError: | ||
| logger.warning( | ||
| "SSL error when downloading %s, retrying with verify=False", | ||
| test_data_path, | ||
| ) | ||
| s3_client = _make_s3_client(s3_creds, verify=False) | ||
| s3_client.download_file(test_data_bucket_name, test_data_path, test_data.path) | ||
| logger.info("Download completed successfully with verify=False") | ||
| except ClientError as e: | ||
| if e.response.get("Error", {}).get("Code") in ("404", "NoSuchKey"): | ||
| raise FileNotFoundError( | ||
| "Test data object not found in S3. bucket=%r, key=%r. " | ||
| "Check that test_data_key (pipeline parameter) is the full object key to an existing JSON file." | ||
| % (test_data_bucket_name, test_data_path) | ||
| ) from e | ||
| else: | ||
| raise TestDataLoaderException("Failed to fetch %s: %s", test_data_path, e) from e | ||
|
|
||
| try: | ||
| with open(test_data.path, "r") as f: | ||
| json.load(f) | ||
| except JSONDecodeError as e: | ||
| raise TestDataLoaderException("test_data_path must point to a valid JSON file.") from e | ||
|
|
||
| get_test_data_s3() | ||
| except Exception as e: | ||
| raise TestDataLoaderException("Failed to fetch %s: %s", test_data_path, e) from e | ||
|
|
There was a problem hiding this comment.
Retry path bypasses error normalization, and custom exception messages are misformatted.
At Lines 79-81, a retry failure escapes the ClientError mapping branch, so NoSuchKey/404 is no longer normalized after SSL fallback. Also at Lines 90 and 92, TestDataLoaderException is raised with tuple args instead of a rendered message.
Suggested patch
- try:
- logger.info(f"Starting download to {test_data.path}")
- s3_client.download_file(test_data_bucket_name, test_data_path, test_data.path)
- logger.info("Download completed successfully")
- except SSLError:
- logger.warning(
- "SSL error when downloading %s, retrying with verify=False",
- test_data_path,
- )
- s3_client = _make_s3_client(s3_creds, verify=False)
- s3_client.download_file(test_data_bucket_name, test_data_path, test_data.path)
- logger.info("Download completed successfully with verify=False")
- except ClientError as e:
+ def _download_once(client):
+ client.download_file(test_data_bucket_name, test_data_path, test_data.path)
+
+ try:
+ logger.info(f"Starting download to {test_data.path}")
+ _download_once(s3_client)
+ logger.info("Download completed successfully")
+ except SSLError:
+ logger.warning("SSL error when downloading %s, retrying with verify=False", test_data_path)
+ try:
+ _download_once(_make_s3_client(s3_creds, verify=False))
+ logger.info("Download completed successfully with verify=False")
+ except ClientError as e:
+ if e.response.get("Error", {}).get("Code") in ("404", "NoSuchKey"):
+ raise FileNotFoundError(
+ "Test data object not found in S3. bucket=%r, key=%r. "
+ "Check that test_data_key (pipeline parameter) is the full object key to an existing JSON file."
+ % (test_data_bucket_name, test_data_path)
+ ) from e
+ raise TestDataLoaderException(f"Failed to fetch {test_data_path}: {e}") from e
+ except Exception as e:
+ raise TestDataLoaderException(f"Failed to fetch {test_data_path}: {e}") from e
+ except ClientError as e:
if e.response.get("Error", {}).get("Code") in ("404", "NoSuchKey"):
raise FileNotFoundError(
"Test data object not found in S3. bucket=%r, key=%r. "
"Check that test_data_key (pipeline parameter) is the full object key to an existing JSON file."
% (test_data_bucket_name, test_data_path)
) from e
else:
- raise TestDataLoaderException("Failed to fetch %s: %s", test_data_path, e) from e
+ raise TestDataLoaderException(f"Failed to fetch {test_data_path}: {e}") from e
except Exception as e:
- raise TestDataLoaderException("Failed to fetch %s: %s", test_data_path, e) from e
+ raise TestDataLoaderException(f"Failed to fetch {test_data_path}: {e}") from e🤖 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 `@components/data_processing/autorag/test_data_loader/component.py` around
lines 74 - 93, The SSLError retry path bypasses the ClientError handling and
also raises TestDataLoaderException with tuple-style args; update the SSLError
except block to wrap the verify=False download call in the same try/except
handling as the primary download (catch ClientError and map 404/NoSuchKey to
FileNotFoundError, re-raise other ClientError and generic Exception as
TestDataLoaderException using a formatted string), i.e., call
_make_s3_client(..., verify=False) then call s3_client.download_file(...) inside
a try/except that mirrors the existing ClientError/Exception branches and use
formatted messages (f-strings or .format) when constructing
TestDataLoaderException so the message is a string and preserve "from e" when
re-raising; reference TestDataLoaderException, _make_s3_client,
s3_client.download_file, ClientError, SSLError, test_data_bucket_name and
test_data_path to locate changes.
LukaszCmielowski
suggested changes
May 19, 2026
LukaszCmielowski
left a comment
LukaszCmielowski
left a comment
There was a problem hiding this comment.
let's keep the aligned structure for variety of templates - move the script template to dedicated folder e.g. script_templates - similar to notebooks
|
|
||
| @dsl.component( | ||
| base_image=AUTORAG_IMAGE, # noqa: E501 | ||
| install_kfp_package=False, |
Author
There was a problem hiding this comment.
By default KFP backend tries installing kfp package before running each component. It is disabled only in the containerised type of components (which wer're currently not using).
We're utilising a base image with every dependency already satisfied, so trying to re-install kfp makes no sense.
|
@filip-komarzyniec let's make sure all checks are passing. |
…teractive script; related changes Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
…ponents Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
components/training/autorag/rag_templates_optimization/tests/test_nested_names.py (1)
49-49: ⚡ Quick winTest isolation broken by modifying module-level state.
Directly assigning
nested_names._ssl_loggerin test methods breaks test isolation. If tests run in parallel or in different order, this shared state can cause interference. Use@patchor a fixture to manage logger configuration.♻️ Use patch decorator for logger isolation
`@patch`("ogx_client.OgxClient") `@patch`("httpx.Client") `@patch`("httpx.get") - def test_self_signed_cert_fallback(self, mock_httpx_get, mock_httpx_client, mock_ogx_client_class): + `@patch`("kfp_components.components.training.autorag.rag_templates_optimization.tests.nested_names._ssl_logger") + def test_self_signed_cert_fallback(self, mock_logger, mock_httpx_get, mock_httpx_client, mock_ogx_client_class): """Test fallback to unverified client when self-signed certificate detected.""" - import logging - - from kfp_components.components.training.autorag.rag_templates_optimization.tests import nested_names - - nested_names._ssl_logger = logging.getLogger("test") - # First call raises ConnectError with self-signed cert messageApply the same pattern to
test_self_signed_cert_fallback_fails.Also applies to: 80-80
🤖 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 `@components/training/autorag/rag_templates_optimization/tests/test_nested_names.py` at line 49, Replace direct assignment to the module-level logger (nested_names._ssl_logger) in the tests with a scoped patch/fixture so tests don't mutate shared state; specifically, use unittest.mock.patch (or pytest monkeypatch/fixture) to temporarily set nested_names._ssl_logger for the test functions (e.g., the test that currently sets nested_names._ssl_logger and test_self_signed_cert_fallback_fails) and ensure the original logger is restored automatically after each test to preserve isolation.
🤖 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.
Inline comments:
In
`@components/training/autorag/rag_templates_optimization/create_model_response.py.templ`:
- Line 47: The assignment "body_template = $response_template" is invalid
Python; change it so the template variable is either a properly quoted/escaped
Python string or replaced at build time with validated content. Update the
templating step to require and validate substitution of $response_template
(failing the build if undefined), ensure the injected value is escaped/validated
to prevent code injection, and if the intent was a literal "$response_template"
use a quoted string; locate the assignment to body_template in
create_model_response.py.templ to apply these fixes.
- Line 67: The code mutates the shared body_template by assigning
body_template["input"] = question each iteration; instead, make a per-request
copy of the template before modifying it (e.g., use a shallow copy or deepcopy
as appropriate) and set the "input" key on that copy, and optionally validate
that the original template does not unexpectedly already include an "input" key;
update the code paths that reference body_template (the variable named
body_template and the surrounding create_model_response logic) to use the
copied_template when sending the request.
- Around line 44-89: The httpx.Client is created without a context manager
(client = httpx.Client(...)) and client.close() is only called on the normal
exit path, causing leaked connections on unhandled exceptions; fix by
instantiating the client with a context manager (with
httpx.Client(verify=verify, timeout=120.0) as client:) and move the interactive
loop and all request/exception handling inside that with-block so the client is
always closed automatically; remove the explicit client.close() and ensure
existing references to client.post and response handling remain unchanged.
In
`@components/training/autorag/rag_templates_optimization/tests/test_nested_names.py`:
- Around line 97-109: The test shows non-SSL connection errors (e.g.
httpx.ConnectError) are currently swallowed by _create_ogx_client; change the
exception handling in _create_ogx_client so it only suppresses SSL-related
errors and not generic network/connectivity errors: catch ssl.SSLError (or check
exception chain for SSL) and handle that case, but allow httpx.ConnectError and
other non-SSL exceptions to be logged and re-raised (or at least not silently
ignored) so callers see the failure; update the health-check/httpx.get error
handling accordingly and keep the existing client-creation call
(mock_ogx_client_class) only after a successful or explicitly-allowed SSL
fallback.
---
Nitpick comments:
In
`@components/training/autorag/rag_templates_optimization/tests/test_nested_names.py`:
- Line 49: Replace direct assignment to the module-level logger
(nested_names._ssl_logger) in the tests with a scoped patch/fixture so tests
don't mutate shared state; specifically, use unittest.mock.patch (or pytest
monkeypatch/fixture) to temporarily set nested_names._ssl_logger for the test
functions (e.g., the test that currently sets nested_names._ssl_logger and
test_self_signed_cert_fallback_fails) and ensure the original logger is restored
automatically after each test to preserve isolation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7ca8f6f4-a5c3-473a-9c2a-212bad74883c
📒 Files selected for processing (3)
components/training/autorag/rag_templates_optimization/create_model_response.py.templcomponents/training/autorag/rag_templates_optimization/tests/test_nested_names.pypipelines/training/autorag/documents_rag_optimization_pipeline/pipeline.py
💤 Files with no reviewable changes (1)
- pipelines/training/autorag/documents_rag_optimization_pipeline/pipeline.py
Comment on lines
+44
to
+89
| client = httpx.Client(verify=verify, timeout=120.0) | ||
|
|
||
| # Request body template | ||
| body_template = $response_template | ||
|
|
||
| # Build headers | ||
| headers = { | ||
| "Content-Type": "application/json", | ||
| "Accept": "application/json", | ||
| } | ||
| if api_key: | ||
| headers["Authorization"] = f"Bearer {api_key}" | ||
|
|
||
| url = f"{base_url}/v1/responses" | ||
|
|
||
| # Interactive loop | ||
| print("Enter questions (Ctrl-D to exit):") | ||
| while True: | ||
| try: | ||
| question = input("Question: ") # TODO nice multiline input with formatting | ||
| if not question: | ||
| continue | ||
|
|
||
| body_template["input"] = question | ||
|
|
||
| try: | ||
| response = client.post(url, json=body_template, headers=headers) | ||
| response.raise_for_status() | ||
| print(response.text) | ||
| except httpx.HTTPStatusError as e: | ||
| print( | ||
| f"HTTP {e.response.status_code}: {e.response.reason_phrase}", | ||
| file=sys.stderr, | ||
| ) | ||
| print(e.response.text, file=sys.stderr) | ||
| except httpx.RequestError as e: | ||
| print(f"Connection error: {e}", file=sys.stderr) | ||
|
|
||
| except EOFError: # Ctrl-D | ||
| print() | ||
| break | ||
| except KeyboardInterrupt: # Ctrl-C | ||
| print() | ||
| break | ||
|
|
||
| client.close() |
There was a problem hiding this comment.
Client resource leak on unhandled exceptions.
The httpx.Client is created outside a context manager and close() is only called at line 89 in the normal exit path. If an unhandled exception occurs between client creation and the EOFError/KeyboardInterrupt handlers (e.g., from the POST request, JSON encoding, or other runtime errors), the client remains open, leaking connections.
♻️ Refactor to use context manager
# Configure HTTP client
verify = get_verify_param()
- client = httpx.Client(verify=verify, timeout=120.0)
+
+ with httpx.Client(verify=verify, timeout=120.0) as client:
+ # Request body template
+ body_template_base = $response_template
- # Request body template
- body_template = $response_template
-
- # Build headers
- headers = {
- "Content-Type": "application/json",
- "Accept": "application/json",
- }
- if api_key:
- headers["Authorization"] = f"Bearer {api_key}"
+ # Build headers
+ headers = {
+ "Content-Type": "application/json",
+ "Accept": "application/json",
+ }
+ if api_key:
+ headers["Authorization"] = f"Bearer {api_key}"
- url = f"{base_url}/v1/responses"
+ url = f"{base_url}/v1/responses"
- # Interactive loop
- print("Enter questions (Ctrl-D to exit):")
- while True:
- try:
- question = input("Question: ") # TODO nice multiline input with formatting
- if not question:
- continue
-
- body_template["input"] = question
-
- try:
- response = client.post(url, json=body_template, headers=headers)
- response.raise_for_status()
- print(response.text)
- except httpx.HTTPStatusError as e:
- print(
- f"HTTP {e.response.status_code}: {e.response.reason_phrase}",
- file=sys.stderr,
- )
- print(e.response.text, file=sys.stderr)
- except httpx.RequestError as e:
- print(f"Connection error: {e}", file=sys.stderr)
-
- except EOFError: # Ctrl-D
- print()
- break
- except KeyboardInterrupt: # Ctrl-C
- print()
- break
-
- client.close()
+ # Interactive loop
+ print("Enter questions (Ctrl-D to exit):")
+ while True:
+ try:
+ question = input("Question: ") # TODO nice multiline input with formatting
+ if not question:
+ continue
+
+ body = {**body_template_base, "input": question}
+
+ try:
+ response = client.post(url, json=body, headers=headers)
+ response.raise_for_status()
+ print(response.text)
+ except httpx.HTTPStatusError as e:
+ print(
+ f"HTTP {e.response.status_code}: {e.response.reason_phrase}",
+ file=sys.stderr,
+ )
+ print(e.response.text, file=sys.stderr)
+ except httpx.RequestError as e:
+ print(f"Connection error: {e}", file=sys.stderr)
+
+ except EOFError: # Ctrl-D
+ print()
+ break
+ except KeyboardInterrupt: # Ctrl-C
+ print()
+ break
+
return 0🤖 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
`@components/training/autorag/rag_templates_optimization/create_model_response.py.templ`
around lines 44 - 89, The httpx.Client is created without a context manager
(client = httpx.Client(...)) and client.close() is only called on the normal
exit path, causing leaked connections on unhandled exceptions; fix by
instantiating the client with a context manager (with
httpx.Client(verify=verify, timeout=120.0) as client:) and move the interactive
loop and all request/exception handling inside that with-block so the client is
always closed automatically; remove the explicit client.close() and ensure
existing references to client.post and response handling remain unchanged.
| client = httpx.Client(verify=verify, timeout=120.0) | ||
|
|
||
| # Request body template | ||
| body_template = $response_template |
There was a problem hiding this comment.
Invalid Python syntax prevents script execution.
$response_template is not valid Python. While the .templ filename suggests template processing, the code cannot execute as written. If this placeholder is substituted at build time, ensure:
- Substitution is mandatory and validated
- The replacement value is properly escaped/validated to prevent injection
- Build failures occur if the template variable is undefined
If this is meant to be a literal string containing $response_template, use proper escaping.
🤖 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
`@components/training/autorag/rag_templates_optimization/create_model_response.py.templ`
at line 47, The assignment "body_template = $response_template" is invalid
Python; change it so the template variable is either a properly quoted/escaped
Python string or replaced at build time with validated content. Update the
templating step to require and validate substitution of $response_template
(failing the build if undefined), ensure the injected value is escaped/validated
to prevent code injection, and if the intent was a literal "$response_template"
use a quoted string; locate the assignment to body_template in
create_model_response.py.templ to apply these fixes.
| if not question: | ||
| continue | ||
|
|
||
| body_template["input"] = question |
There was a problem hiding this comment.
Unsafe mutation of shared template dictionary.
Mutating body_template["input"] in place on each iteration is error-prone. If $response_template already contains an "input" key, it's silently overwritten without validation. If the template structure changes, this code fails silently.
🔧 Proposed fix to copy template per request
- body_template = $response_template
+ body_template_base = $response_template- body_template["input"] = question
+ body = {**body_template_base, "input": question}
try:
- response = client.post(url, json=body_template, headers=headers)
+ response = client.post(url, json=body, headers=headers)🤖 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
`@components/training/autorag/rag_templates_optimization/create_model_response.py.templ`
at line 67, The code mutates the shared body_template by assigning
body_template["input"] = question each iteration; instead, make a per-request
copy of the template before modifying it (e.g., use a shallow copy or deepcopy
as appropriate) and set the "input" key on that copy, and optionally validate
that the original template does not unexpectedly already include an "input" key;
update the code paths that reference body_template (the variable named
body_template and the surrounding create_model_response logic) to use the
copied_template when sending the request.
Comment on lines
+97
to
+109
| def test_non_ssl_error_handled_gracefully(self, mock_httpx_get, mock_ogx_client_class): | ||
| """Test that non-SSL errors are swallowed and client is created anyway.""" | ||
| mock_httpx_get.side_effect = httpx.ConnectError("Connection timeout") | ||
| mock_client = Mock() | ||
| mock_ogx_client_class.return_value = mock_client | ||
|
|
||
| result = _create_ogx_client("https://ogx.example.com", "test-api-key") | ||
|
|
||
| # Non-SSL errors are swallowed, client is created normally | ||
| assert result == mock_client | ||
| mock_ogx_client_class.assert_called_once_with( | ||
| base_url="https://ogx.example.com", api_key="test-api-key" | ||
| ) |
There was a problem hiding this comment.
Test validates unreliable error-swallowing behavior.
This test confirms that non-SSL connection errors (timeouts, network failures) are silently ignored during the health check, and the client is created anyway. While the test itself is correct, it validates problematic behavior: if connectivity is broken during the initial check, proceeding to create the client masks the real issue and will likely cause confusing failures later when the client is actually used. The implementation (not this test) should either propagate non-SSL connection errors or log them prominently.
🤖 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
`@components/training/autorag/rag_templates_optimization/tests/test_nested_names.py`
around lines 97 - 109, The test shows non-SSL connection errors (e.g.
httpx.ConnectError) are currently swallowed by _create_ogx_client; change the
exception handling in _create_ogx_client so it only suppresses SSL-related
errors and not generic network/connectivity errors: catch ssl.SSLError (or check
exception chain for SSL) and handle that case, but allow httpx.ConnectError and
other non-SSL exceptions to be logged and re-raised (or at least not silently
ignored) so callers see the failure; update the health-check/httpx.get error
handling accordingly and keep the existing client-creation call
(mock_ogx_client_class) only after a successful or explicitly-allowed SSL
fallback.
|
Please do not merge the changes, until this PR has also new version of ai4rag included. |
…nse` interactive script; related changes Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Signed-off-by: Filip Komarzyniec <fkomarzy@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
components/training/autorag/rag_templates_optimization/script_templates/create_model_response.py.templ (2)
44-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHTTP client lifetime is not exception-safe.
If an unexpected exception escapes the loop,
client.close()is skipped and connections remain open. Use a context manager for deterministic cleanup.Suggested fix
- client = httpx.Client(verify=verify, timeout=120.0) + with httpx.Client(verify=verify, timeout=120.0) as client: @@ - print("Enter questions (Ctrl-D to exit):") - while True: + print("Enter questions (Ctrl-D to exit):") + while True: @@ - client.close() return 0#!/bin/bash # Verify non-context-managed httpx client usage in this script template. set -euo pipefail rg -n -C 3 'httpx\.Client\(|client\.close\(|with httpx\.Client' components/training/autorag/rag_templates_optimization/script_templates/create_model_response.py.templAs per coding guidelines, "REVIEW PRIORITIES: 3. Bug-prone patterns and error handling gaps".
Also applies to: 89-89
🤖 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 `@components/training/autorag/rag_templates_optimization/script_templates/create_model_response.py.templ` around lines 44 - 45, Replace the bare httpx.Client instantiation with a context-managed client to ensure deterministic cleanup: locate the httpx.Client(...) call that assigns to the variable client and the later client.close() usage (e.g., the client variable in create_model_response.py.templ), then rewrite the surrounding code to use "with httpx.Client(verify=..., timeout=...) as client:" so the client is automatically closed even if exceptions escape the loop; remove any manual client.close() calls after switching to the with-statement.
47-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTemplate placeholder is executable code injection surface (CWE-94).
body_template = $response_templateexecutes whatever gets substituted. If template input is attacker-controlled (or malformed), this can trigger arbitrary code execution when the generated script runs. Also, missing substitution yields invalid Python and hard failure.Suggested hardening diff
+import json @@ - body_template = $response_template + raw_template = r'''$response_template_json''' + try: + body_template = json.loads(raw_template) + except json.JSONDecodeError as exc: + print(f"Invalid response template JSON: {exc}", file=sys.stderr) + return 1 + if not isinstance(body_template, dict): + print("Response template must decode to a JSON object", file=sys.stderr) + return 1#!/bin/bash # Verify whether substitution is guaranteed and where template input originates. set -euo pipefail rg -n --fixed-strings '$response_template' -C 3 rg -n --fixed-strings '$response_template_json' -C 3 rg -n -C 3 'create_model_response\.py\.templ|script_templates|rag_templates_optimization'As per coding guidelines, "REVIEW PRIORITIES: 1. Security vulnerabilities (provide severity, exploit scenario, and remediation code)".
🤖 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 `@components/training/autorag/rag_templates_optimization/script_templates/create_model_response.py.templ` at line 47, The current template assigns body_template = $response_template which allows arbitrary code injection and can produce invalid Python if substitution is missing; replace the direct executable substitution by requiring and using a safely-encoded string (e.g. use a JSON-escaped variable like $response_template_json and deserialize it at runtime with json.loads) or otherwise wrap the incoming template as a raw string literal with proper escaping, validate the value is present and of expected type before use, and fail-fast with a clear error if validation fails; update references to body_template and any callers of create_model_response.py.templ to use the safe deserialization/validation path rather than direct code substitution.components/training/autorag/rag_templates_optimization/component.py (1)
1-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAnchor embedded-artifact copies to this module, not the process CWD.
These source paths only resolve when import/compile happens from the repo root. Any other working directory will raise
FileNotFoundErrorbefore the component is even defined.Suggested fix
import tempfile +from pathlib import Path from shutil import copy, copytree +_COMPONENT_DIR = Path(__file__).resolve().parent temp_embedded_artifacts_dir = tempfile.TemporaryDirectory() copytree( - "components/training/autorag/rag_templates_optimization/notebook_templates", + _COMPONENT_DIR / "notebook_templates", temp_embedded_artifacts_dir.name, dirs_exist_ok=True, ) copy( - "components/training/autorag/rag_templates_optimization/script_templates/create_model_response.py.templ", + _COMPONENT_DIR / "script_templates" / "create_model_response.py.templ", temp_embedded_artifacts_dir.name, )🤖 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 `@components/training/autorag/rag_templates_optimization/component.py` around lines 1 - 16, The file uses literal relative source paths that only work when running from the repo root; change the copy/copytree sources to be anchored to this module's directory by resolving a base_dir from Path(__file__).resolve().parent (or using importlib.resources) and join the template paths to that base_dir before calling copytree and copy so they won't raise FileNotFoundError when the process CWD is different; update references around temp_embedded_artifacts_dir, copytree and copy to use those resolved source paths.
🤖 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.
Inline comments:
In `@components/training/autorag/rag_templates_optimization/component.py`:
- Around line 614-616: Validate and sanitize
pattern_data["payload"]["pattern_name"] before constructing patt_dir to prevent
path traversal: ensure the value is not empty, not absolute (no leading "/" or
drive letters), does not contain path separators or "..", and matches an allowed
filename pattern (e.g., alphanumeric, underscores, hyphens, dots) or use
Path(...).name to get a basename; if validation fails, log or raise and skip
creating the directory under rag_patterns_dir, otherwise build patt_dir =
rag_patterns_dir / sanitized_name and call mkdir(parents=True, exist_ok=True).
Ensure references: pattern_data, pattern_data["payload"]["pattern_name"],
patt_dir, rag_patterns_dir.
- Around line 118-133: The except block catching httpx.ConnectError (except
httpx.ConnectError as e) currently only handles self-signed-certificate cases
and then falls through, causing a normal OgxClient to be returned on other
connection failures; modify the handler so that after checking for the
self-signed cert pattern you re-raise the original ConnectError for all
non-certificate matches (i.e., add an explicit raise e in the else path of the
except), ensuring only the self-signed branch can return a client and all other
connection errors propagate.
In `@pyproject.toml`:
- Around line 130-132: The package-data entry for
"kfp_components.components.training.autorag.rag_templates_optimization"
currently only includes "notebook_templates/*.ipynb" and misses the new script
template; update the package-data for that key to also include the
script_templates (e.g., add "script_templates/*.templ" or explicitly include
"script_templates/create_model_response.py.templ") so the template is bundled
into wheels/sdists and available at runtime for rag_templates_optimization.
- Line 13: Add httpx as an explicit project dependency by adding it to the
dependencies array in pyproject.toml (alongside the existing "kfp" and
"kfp-kubernetes" entries) — e.g. "httpx>=0.23.0" (choose the appropriate minimum
supported version for your runtime). After updating the dependencies key,
regenerate the lockfile/poetry.lock (or run your dependency resolver) so the new
dependency is pinned and available to runtime components that import httpx
(search_space_preparation, rag_templates_optimization, documents_indexing).
---
Duplicate comments:
In `@components/training/autorag/rag_templates_optimization/component.py`:
- Around line 1-16: The file uses literal relative source paths that only work
when running from the repo root; change the copy/copytree sources to be anchored
to this module's directory by resolving a base_dir from
Path(__file__).resolve().parent (or using importlib.resources) and join the
template paths to that base_dir before calling copytree and copy so they won't
raise FileNotFoundError when the process CWD is different; update references
around temp_embedded_artifacts_dir, copytree and copy to use those resolved
source paths.
In
`@components/training/autorag/rag_templates_optimization/script_templates/create_model_response.py.templ`:
- Around line 44-45: Replace the bare httpx.Client instantiation with a
context-managed client to ensure deterministic cleanup: locate the
httpx.Client(...) call that assigns to the variable client and the later
client.close() usage (e.g., the client variable in
create_model_response.py.templ), then rewrite the surrounding code to use "with
httpx.Client(verify=..., timeout=...) as client:" so the client is automatically
closed even if exceptions escape the loop; remove any manual client.close()
calls after switching to the with-statement.
- Line 47: The current template assigns body_template = $response_template which
allows arbitrary code injection and can produce invalid Python if substitution
is missing; replace the direct executable substitution by requiring and using a
safely-encoded string (e.g. use a JSON-escaped variable like
$response_template_json and deserialize it at runtime with json.loads) or
otherwise wrap the incoming template as a raw string literal with proper
escaping, validate the value is present and of expected type before use, and
fail-fast with a clear error if validation fails; update references to
body_template and any callers of create_model_response.py.templ to use the safe
deserialization/validation path rather than direct code substitution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 40891393-d32b-4a73-9f66-bff33ebbbb2c
📒 Files selected for processing (3)
components/training/autorag/rag_templates_optimization/component.pycomponents/training/autorag/rag_templates_optimization/script_templates/create_model_response.py.templpyproject.toml
Comment on lines
118
to
+133
| try: | ||
| client.models.list() | ||
| except (ssl.SSLCertVerificationError, httpx.ConnectError, OGXAPIConnectionError) as exc: | ||
| if _is_ssl_error(exc): | ||
| _ssl_logger.warning("SSL verification failed for OgxClient — retrying with verify=False. ") | ||
| client = OgxClient( | ||
| **kwargs, | ||
| http_client=httpx.Client(verify=False), | ||
| ) | ||
| _ogx_ssl_verify[0] = False | ||
| else: | ||
| raise | ||
| return client | ||
| httpx.get(base_url) | ||
| except httpx.ConnectError as e: | ||
| if search(r"\bself.*signed.*certificate\b", str(e)): | ||
| _ssl_logger.info("OGX server presents a self-signed certificate") | ||
| if httpx.get(base_url, verify=False).status_code != 200: | ||
| _ssl_logger.error( | ||
| "Cannot establish connection with the OGX server even without " | ||
| "verification of the self-signed certificate.", | ||
| exc_info=True, | ||
| ) | ||
| raise e | ||
| _ssl_logger.warning("Initialising OGXClient without server-side certificate verification.") | ||
| return OgxClient(base_url=base_url, api_key=api_key, http_client=httpx.Client(verify=False)) | ||
|
|
||
| return OgxClient(base_url=base_url, api_key=api_key) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the specific file
find . -type f -name "component.py" -path "*/training/autorag/rag_templates_optimization/*"Repository: opendatahub-io/pipelines-components
Length of output: 149
🏁 Script executed:
# Read the file to verify the code at lines 118-133
cat -n components/training/autorag/rag_templates_optimization/component.py | sed -n '100,150p'Repository: opendatahub-io/pipelines-components
Length of output: 3036
🏁 Script executed:
# Get more context around the function to understand the full intent
cat -n components/training/autorag/rag_templates_optimization/component.py | sed -n '80,160p'Repository: opendatahub-io/pipelines-components
Length of output: 4630
🏁 Script executed:
# Check if there are additional except handlers after the ConnectError handler
cat -n components/training/autorag/rag_templates_optimization/component.py | sed -n '118,135p'Repository: opendatahub-io/pipelines-components
Length of output: 1197
🏁 Script executed:
# Verify httpx.ConnectError documentation and what errors it catches
rg "ConnectError" components/training/autorag/rag_templates_optimization/component.py -B 2 -A 2Repository: opendatahub-io/pipelines-components
Length of output: 311
🏁 Script executed:
# Check if there are other usages of this function to understand intent
rg "_create_ogx_client" components/training/autorag/rag_templates_optimization/component.py -B 2 -A 2Repository: opendatahub-io/pipelines-components
Length of output: 414
Re-raise non-certificate ConnectErrors to prevent silent failures.
When httpx.get(base_url) fails with a ConnectError that does not match the self-signed certificate pattern (DNS failures, connection refused, timeouts, etc.), the exception handler falls through without raising or returning. This causes the function to return a normal OgxClient despite the connection failure, masking network issues until much later in pipeline execution.
try:
httpx.get(base_url)
except httpx.ConnectError as e:
if search(r"\bself.*signed.*certificate\b", str(e)):
_ssl_logger.info("OGX server presents a self-signed certificate")
if httpx.get(base_url, verify=False).status_code != 200:
_ssl_logger.error(
"Cannot establish connection with the OGX server even without "
"verification of the self-signed certificate.",
exc_info=True,
)
raise e
_ssl_logger.warning("Initialising OGXClient without server-side certificate verification.")
return OgxClient(base_url=base_url, api_key=api_key, http_client=httpx.Client(verify=False))
+ raise
return OgxClient(base_url=base_url, api_key=api_key)🤖 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 `@components/training/autorag/rag_templates_optimization/component.py` around
lines 118 - 133, The except block catching httpx.ConnectError (except
httpx.ConnectError as e) currently only handles self-signed-certificate cases
and then falls through, causing a normal OgxClient to be returned on other
connection failures; modify the handler so that after checking for the
self-signed cert pattern you re-raise the original ConnectError for all
non-certificate matches (i.e., add an explicit raise e in the else path of the
except), ensuring only the self-signed branch can return a client and all other
connection errors propagate.
Comment on lines
+614
to
616
| for pattern_data in rag_exp.event_handler.patterns: | ||
| patt_dir = rag_patterns_dir / pattern_data["payload"]["pattern_name"] | ||
| patt_dir.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
Validate pattern_name before using it as a directory name.
pattern_data["payload"]["pattern_name"] is written straight into the filesystem path. If it contains /, \, .., or an absolute path, it can create unexpected nested output or escape rag_patterns_dir entirely (CWE-22).
Suggested fix
for pattern_data in rag_exp.event_handler.patterns:
- patt_dir = rag_patterns_dir / pattern_data["payload"]["pattern_name"]
+ pattern_name = str(pattern_data["payload"]["pattern_name"]).strip()
+ if (
+ not pattern_name
+ or pattern_name in {".", ".."}
+ or Path(pattern_name).is_absolute()
+ or search(r"[\\/]", pattern_name)
+ ):
+ raise ValueError(f"Unsafe pattern_name: {pattern_name!r}")
+ patt_dir = rag_patterns_dir / pattern_name
patt_dir.mkdir(parents=True, exist_ok=True)As per coding guidelines "Validate file paths (prevent path traversal)".
🤖 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 `@components/training/autorag/rag_templates_optimization/component.py` around
lines 614 - 616, Validate and sanitize pattern_data["payload"]["pattern_name"]
before constructing patt_dir to prevent path traversal: ensure the value is not
empty, not absolute (no leading "/" or drive letters), does not contain path
separators or "..", and matches an allowed filename pattern (e.g., alphanumeric,
underscores, hyphens, dots) or use Path(...).name to get a basename; if
validation fails, log or raise and skip creating the directory under
rag_patterns_dir, otherwise build patt_dir = rag_patterns_dir / sanitized_name
and call mkdir(parents=True, exist_ok=True). Ensure references: pattern_data,
pattern_data["payload"]["pattern_name"], patt_dir, rag_patterns_dir.
| "kfp>=2.16.1", | ||
| "kfp-kubernetes>=2.16.1", | ||
| ] | ||
| dependencies = ["kfp>=2.16.1", "kfp-kubernetes>=2.16.1"] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify httpx usage vs dependency declarations.
set -euo pipefail
rg -n -C 2 '^\s*import httpx|httpx\.' components
rg -n -C 2 '^\s*dependencies\s*=|httpx' pyproject.toml requirements.txt requirements-dev.txt 2>/dev/null || trueRepository: opendatahub-io/pipelines-components
Length of output: 33468
Add httpx as an explicit dependency in pyproject.toml.
httpx is imported directly across multiple runtime components (search_space_preparation, rag_templates_optimization, documents_indexing) and templates, but is not declared as a direct dependency. Transitive dependency chains can break.
🤖 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 `@pyproject.toml` at line 13, Add httpx as an explicit project dependency by
adding it to the dependencies array in pyproject.toml (alongside the existing
"kfp" and "kfp-kubernetes" entries) — e.g. "httpx>=0.23.0" (choose the
appropriate minimum supported version for your runtime). After updating the
dependencies key, regenerate the lockfile/poetry.lock (or run your dependency
resolver) so the new dependency is pinned and available to runtime components
that import httpx (search_space_preparation, rag_templates_optimization,
documents_indexing).
Comment on lines
+130
to
+132
| "kfp_components.components.training.autorag.rag_templates_optimization" = [ | ||
| "notebook_templates/*.ipynb", | ||
| ] |
There was a problem hiding this comment.
Include script templates in package data for rag_templates_optimization.
The new script_templates/create_model_response.py.templ artifact is not included in tool.setuptools.package-data. Wheels/sdists can omit it, breaking runtime template generation.
Suggested fix
"kfp_components.components.training.autorag.rag_templates_optimization" = [
"notebook_templates/*.ipynb",
+ "script_templates/*.templ",
]As per coding guidelines, "REVIEW PRIORITIES: 2. Architectural issues and anti-patterns".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "kfp_components.components.training.autorag.rag_templates_optimization" = [ | |
| "notebook_templates/*.ipynb", | |
| ] | |
| "kfp_components.components.training.autorag.rag_templates_optimization" = [ | |
| "notebook_templates/*.ipynb", | |
| "script_templates/*.templ", | |
| ] |
🤖 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 `@pyproject.toml` around lines 130 - 132, The package-data entry for
"kfp_components.components.training.autorag.rag_templates_optimization"
currently only includes "notebook_templates/*.ipynb" and misses the new script
template; update the package-data for that key to also include the
script_templates (e.g., add "script_templates/*.templ" or explicitly include
"script_templates/create_model_response.py.templ") so the template is bundled
into wheels/sdists and available at runtime for rag_templates_optimization.
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
3 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.
fixes: RHOAIENG-60206
Description of your changes:
ai4raglibraryresponses_api_artifactartifactcreate_responses_model.pyscript for each pattern generated as part ofrag_patternsartifact,Checklist:
Pre-Submission Checklist
Learn more about the pull request title convention used in this repository.
Additional Checklist Items for New or Updated Components/Pipelines
metadata.yamlincludes freshlastVerifiedtimestampare present and complete
snake_casenaming conventionSummary by CodeRabbit
Release Notes
Removals
prepare_responses_api_requestsdeployment component and related response API request body generation functionality.Improvements
Refactoring