feat: add agentic CPT projects (mcp_gateway, llamastack, agentic_tools)#75
Conversation
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds shared agentic_tools runtime and Locust/MCP utilities, extends Caliper metrics capture and MLflow export with workspace and multi-run support, and introduces MCP Gateway orchestration, install/cleanup tooling, CI phases, and post-processing components. ChangesAgentic Tools and Shared Runtime
Caliper Metrics and Export
MCP Gateway Orchestration and Tooling
Sequence Diagram(s)sequenceDiagram
participant ci_py as ci.py
participant prepare as prepare_phase.run()
participant test as test_phase.run()
participant locust as run_distributed.run()
participant metrics as capture_metrics()
participant cleanup as cleanup_phase.run()
ci_py->>prepare: prepare
prepare->>prepare: clone platform repo and install platform
prepare-->>ci_py: 0
ci_py->>test: test
loop servers × concurrency × targets
test->>locust: run(locust kwargs)
locust-->>test: LocustResults
test->>metrics: capture_metrics()
metrics-->>test: metrics artifacts
end
test-->>ci_py: 0
ci_py->>cleanup: post-cleanup
cleanup->>cleanup: delete test resources and platform
cleanup-->>ci_py: 0
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
c898bbc to
d61c884
Compare
| requested_preset = os.environ.get("FORGE_PRESET") | ||
| if requested_preset: | ||
| if not config.project.get_preset(requested_preset): | ||
| if phase and phase in _preset_optional: | ||
| logger.info( | ||
| "Preset '%s' not found, but not required for '%s'", requested_preset, phase | ||
| ) | ||
| else: | ||
| raise ValueError(f"Unknown {project_name} preset: {requested_preset}") | ||
| else: | ||
| config.project.apply_preset(requested_preset) | ||
| config.project.apply_config_overrides(log=False) |
There was a problem hiding this comment.
this is already done by config.init
| def _init(phase: str | None = None): | ||
| import os | ||
|
|
||
| env.init() | ||
| run.init() | ||
| config.init(config_dir) |
There was a problem hiding this comment.
sometimes we need more flexibility than that 🤔
| logger.info(f"No vaults to initialize for phase '{phase}'") | ||
| return | ||
|
|
||
| vault.init(mandatory_vaults=mandatory_vaults, optional_vaults=optional_vaults) |
There was a problem hiding this comment.
again, I like the idea, but sometimes we'll need more flexibility
like:
- if I run with the preset
test_from_hf_models, I want the HF vault - if I run with the preset
test_from_ibm_models, I want the IBM vault (that's a use case we had in the past with fine-tuning)
if you're not flexible, you request the HF vault and the IBM vault, and people complain because they want to use HF and don't care about the IBM secrets ...
| func_path = phase_spec["func"] | ||
| help_text = phase_spec.get("help") | ||
|
|
||
| _register_phase_command(main, cmd_name, func_path, help_text) |
There was a problem hiding this comment.
problem with this approach is that it mandates every project to work in a lock step.
eg, here, I want to introduce a new feature to review the failure with an agent
@agent_review_on_failure
def test(ctx) -> int:
but I don't want to force it to all the tests. Only llm-d for the time being. And the fournos-launcher project will never want it.
that's the hard aspect of sharing vs not sharing :/
| oc( | ||
| "patch", | ||
| "llamastackdistribution", | ||
| distribution_name, | ||
| "-n", | ||
| namespace, | ||
| "--type", | ||
| "merge", | ||
| "-p", | ||
| f'{{"spec":{{"replicas":{replicas}}}}}', | ||
| check=True, | ||
| ) |
There was a problem hiding this comment.
isn't that oc scale llamastackdistribution/NAME --replicas={replicas}?
There was a problem hiding this comment.
and I think the rollout isn't necessary after oc scale (I never use that 🙃)
| oc( | ||
| "patch", | ||
| "datasciencecluster", | ||
| "default-dsc", | ||
| "--type", | ||
| "merge", | ||
| "-p", | ||
| '{"spec":{"components":{"llamastackoperator":{"managementState":"Managed"}}}}', | ||
| check=False, | ||
| ) |
There was a problem hiding this comment.
I'm surprised to see that after oc patch llamastackdistribution 🤔
| import time | ||
|
|
||
| time.sleep(30) | ||
| return "LlamaStack operator verified" |
There was a problem hiding this comment.
no sleep 30 please (unreliable),
wait for the DSC to turn READY
there's already a toolbox script to update the DSC, you can either use it or take the task that waits for the DSC readiness
| from projects.core.dsl import always, entrypoint, execute_tasks, retry, task | ||
| from projects.core.dsl.utils.k8s import oc, oc_get_json, oc_resource_exists | ||
| from projects.core.library import env | ||
| from projects.llamastack.orchestration.runtime_config import cfg |
There was a problem hiding this comment.
you should pass all the relevant information through the run() entrypoint, for a strong isolation
(the toolbox script should be standalone, for easy reuse and troubleshooting)
| min_replicas = hpa_cfg.get("min_replicas", 1) | ||
| max_replicas = hpa_cfg.get("max_replicas", 4) | ||
| memory_target = hpa_cfg.get("memory_target", 75) | ||
|
|
There was a problem hiding this comment.
should go away (see above)
should go to the run() prototype: accept min/max_replicas/memory_target with default values
| time.sleep(15) | ||
| return f"HPA configured (min={min_replicas}, max={max_replicas}, mem={memory_target}%)" |
There was a problem hiding this comment.
no sleep, need to wait (maybe in another task) for the llamastackdistribution to reflect the update, one way or another
| manifests_dir = cfg.get_manifests_dir() | ||
| manifest_path = manifests_dir / "postgres.yaml" |
There was a problem hiding this comment.
the manifests should go to this deploy_postgres directory, in a manifest directory, or perhaps templates
| Deploy RHAIIS/vLLM inference service for Llama Stack testing. | ||
|
|
||
| Migrated from tekton-benchmarks/tasks/deploy-rhaiis.yaml. | ||
| Handles: | ||
| - Creating PVC for model weights (if needed) | ||
| - Deploying ServingRuntime and InferenceService via KServe | ||
| - Waiting for the inference service to be ready |
There was a problem hiding this comment.
Mehul should have provided toolbox scripts for that
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟡 Minor comments (6)
projects/agentic_tools/utils/token_text.py-56-67 (1)
56-67:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate inputs before sampling and guarantee
textis initialized.
build_exact_textcan crash on edge inputs (max_attempts <= 0→ unboundtext; emptyvalid_ids→ sampling failure). Fail fast with explicit validation.Suggested fix
def build_exact_text( tokenizer: Any, valid_ids: list[int], num_tokens: int, prefix: str = "", max_attempts: int = 10, ) -> str: @@ - for _attempt in range(max_attempts): + if num_tokens <= 0: + raise ValueError("num_tokens must be > 0") + if max_attempts <= 0: + raise ValueError("max_attempts must be > 0") + if not valid_ids: + raise ValueError("valid_ids must not be empty") + + text = prefix + for _attempt in range(max_attempts): filler_ids = random.choices(valid_ids, k=num_tokens * 2) text = prefix + tokenizer.decode(filler_ids, skip_special_tokens=True) @@ return text🤖 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 `@projects/agentic_tools/utils/token_text.py` around lines 56 - 67, Add input validation at the start of the build_exact_text function to check that max_attempts is greater than zero and valid_ids is not empty; if either condition fails, raise an appropriate error. Additionally, initialize the text variable before the for loop over max_attempts to ensure it is always defined when the function returns, preventing an UnboundLocalError if the loop never executes or the conditions are not met.projects/mcp_gateway/orchestration/runtime_config.py-41-43 (1)
41-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve explicit
spawn_rate=0instead of treating it as unset.Using truthiness here drops
0toNone, which silently changes runtime behavior.💡 Suggested fix
- return int(raw) if raw else None + return int(raw) if raw is not None else None🤖 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 `@projects/mcp_gateway/orchestration/runtime_config.py` around lines 41 - 43, The get_spawn_rate method uses a truthiness check on the raw value which incorrectly treats 0 as falsy and converts it to None, losing the explicit spawn_rate=0 configuration. Replace the truthiness check with an explicit check for None (e.g., if raw is not None) to preserve 0 as a valid spawn_rate value instead of silently dropping it to None.projects/mcp_gateway/toolbox/platform_helpers.py-246-258 (1)
246-258:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTemporary payload file should be removed after finalizer replacement.
At Line 246,
NamedTemporaryFile(..., delete=False)creates a persistent file, but there is no cleanup after Line 250-Line 257. Repeated runs can accumulate temp files on disk.Suggested fix
with tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False) as f: f.write(payload) tmp_path = f.name - oc( - "replace", - "--raw", - f"/api/v1/namespaces/{namespace}/finalize", - "-f", - tmp_path, - check=False, - ) + try: + oc( + "replace", + "--raw", + f"/api/v1/namespaces/{namespace}/finalize", + "-f", + tmp_path, + check=False, + ) + finally: + Path(tmp_path).unlink(missing_ok=True) logger.info("Removed finalizers from namespace %s", namespace)🤖 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 `@projects/mcp_gateway/toolbox/platform_helpers.py` around lines 246 - 258, The temporary file created with NamedTemporaryFile(..., delete=False) and stored in tmp_path is never cleaned up after being used in the oc() function call, causing temp files to accumulate on disk over repeated runs. After the oc() call completes (following the logger.info statement), explicitly remove the temporary file using os.remove(tmp_path). Wrap this cleanup in a try/finally block or similar error handling to ensure the file is deleted even if the oc() call raises an exception.projects/core/dsl/utils/k8s.py-279-286 (1)
279-286:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTreat successful empty pod logs as success, not failure.
On Line 279, the code marks log capture as failed when
stdoutis empty, even ifreturncode == 0. Pods with no output should still produce a valid (possibly empty) log file.Suggested fix
- if log_result.returncode == 0 and log_result.stdout: - log_file.write_text(log_result.stdout, encoding="utf-8") + if log_result.returncode == 0: + log_file.write_text(log_result.stdout or "", encoding="utf-8") else: log_file.write_text( f"(failed to collect logs: exit_code={log_result.returncode})\n" f"{log_result.stderr or ''}", encoding="utf-8", )🤖 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 `@projects/core/dsl/utils/k8s.py` around lines 279 - 286, The condition on line 279 in the log writing logic incorrectly treats successful commands with empty output as failures. The condition currently checks both returncode == 0 AND log_result.stdout being non-empty, but it should only check if returncode == 0 to consider a pod's empty logs as successful. Modify the if statement to remove the "and log_result.stdout" check so that any command with a successful exit code (returncode == 0) writes the stdout to the log file, regardless of whether the output is empty or not.projects/mcp_gateway/README.md-7-41 (1)
7-41:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language identifier to the fenced code block.
Line 7 opens a fenced block without a language, which matches the MD040 lint warning.
Suggested patch
-``` +```text projects/ ├── agentic_tools/ # Shared toolbox ... -``` +```🤖 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 `@projects/mcp_gateway/README.md` around lines 7 - 41, The fenced code block starting on line 7 of the README.md file is missing a language identifier, which triggers an MD040 linting warning. Add the language identifier `text` immediately after the opening triple backticks (```) to properly declare the code block format and resolve the lint warning.Source: Linters/SAST tools
projects/mcp_gateway/README.md-82-86 (1)
82-86:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winScale-out usage example conflicts with prepare-phase requirements.
Line 82 says
MCP_GATEWAY_VERSIONis not needed, butprepare_phase.run()hard-fails when it is unset (Line 26 inprojects/mcp_gateway/orchestration/prepare_phase.py).Suggested doc fix
-# Scale-out (latest version, no MCP_GATEWAY_VERSION needed) +# Scale-out +export MCP_GATEWAY_VERSION=0.7.0 export FORGE_PRESET=scale-out python -m projects.mcp_gateway.orchestration.ci prepare🤖 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 `@projects/mcp_gateway/README.md` around lines 82 - 86, The scale-out usage example in the README incorrectly states that MCP_GATEWAY_VERSION is not needed, but the prepare_phase.run() function in prepare_phase.py requires this environment variable to be set and will fail without it. Update the scale-out example section in the README to either clarify that MCP_GATEWAY_VERSION is required, or add an export statement for MCP_GATEWAY_VERSION before the prepare, test, and post-cleanup command calls to match the actual requirements of the prepare_phase.run() function.
🧹 Nitpick comments (1)
projects/agentic_tools/locust/toolbox/generate_prompts/main.py (1)
271-271: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winPin generator-script dependency versions for reproducible runs.
Installing floating
transformers/kubernetesversions at runtime makes benchmark behavior nondeterministic and can break unexpectedly over time.Suggested fix
-subprocess.check_call([sys.executable, "-m", "pip", "install", "-q", "transformers", "kubernetes"], stdout=subprocess.DEVNULL) +subprocess.check_call( + [ + sys.executable, + "-m", + "pip", + "install", + "-q", + "transformers==4.52.4", + "kubernetes==30.1.0", + ], + stdout=subprocess.DEVNULL, +)🤖 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 `@projects/agentic_tools/locust/toolbox/generate_prompts/main.py` at line 271, In the subprocess.check_call() command that installs the transformers and kubernetes packages, replace the package names with version-pinned specifications (e.g., transformers==VERSION and kubernetes==VERSION) instead of using floating versions. This ensures reproducible benchmark runs by guaranteeing the same package versions are installed each time the script executes, preventing nondeterministic behavior and unexpected breakage from upstream dependency changes.
🤖 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 `@projects/agentic_tools/mcp/toolbox/deploy_tokenized_mcp_server/main.py`:
- Around line 114-123: The label_str variable is being constructed as a
comma-separated string on a single line, which when inserted into the YAML
template breaks YAML mapping semantics and generates invalid YAML. Instead of
joining labels with commas, format them as separate YAML key-value pairs each on
its own line with proper indentation that aligns with the YAML structure. Modify
the construction of label_str to use newline characters and appropriate spacing
so that each label from labels.items() appears as a properly indented line
within the labels mapping in the manifest template. Apply this same fix to the
other location mentioned in the comment at lines 169-173.
---
Major comments:
In `@projects/agentic_tools/base_runtime_config.py`:
- Around line 95-101: The function returns namespace configuration values
(runtime.namespace_override and runtime.namespace) without validating them
against Kubernetes naming requirements. Add validation logic before returning
either the override variable or the namespace variable that checks if the
returned value conforms to valid Kubernetes namespace naming conventions, and
raise a clear exception if validation fails. This ensures invalid namespace
configurations are caught immediately rather than failing later during command
execution.
In `@projects/agentic_tools/ci_base.py`:
- Around line 126-133: The code at line 130 calls .items() on vault_config
without first verifying it is a dictionary, which will crash if vault_config is
None or a non-dict type. Add a type guard check (using isinstance with dict
type) after the existing list check to ensure vault_config is a dictionary
before attempting to call .items() on it. If vault_config is not a dictionary
and not a list, return an empty list to provide safe default behavior.
In `@projects/agentic_tools/locust/locust_runtime/locust_shapes.py`:
- Around line 71-72: In the load shape calculation at line 71, add a guard
condition to check if ramp_up_end is zero or if the run has zero duration before
performing the division in the expression that calculates current_users. If
ramp_up_end is zero or the run duration would cause a division by zero scenario,
return a shape response that cleanly ends the test (such as returning the
current state or a minimal user count) instead of proceeding with the division
operation. This prevents the ZeroDivisionError crash when RUN_TIME_SECONDS or
DURATION is set to zero.
- Line 92: The PoissonShape class's current_users calculation on line 92 uses
users * 2 as the maximum bound in the random.randint call, which allows a +100%
load increase and violates the documented ±50% load envelope. Change the upper
bound from users * 2 to users * 1.5 to properly enforce the declared ±50%
behavior while keeping the lower bound of max(1, users // 2) unchanged.
- Around line 101-103: The code at lines 101-103 lacks error handling for
malformed JSON in the CUSTOM_STAGES environment variable. When json.loads() is
called on invalid JSON, it raises a JSONDecodeError that crashes during class
initialization. Wrap the json.loads(stages_json) call in a try-except block that
catches JSONDecodeError, and when caught, either log a warning about the
malformed JSON and set self.stages to an empty list as a fallback, or provide a
sensible default value to ensure the shape loading and run startup can continue
gracefully.
In `@projects/agentic_tools/locust/locust_runtime/locustfile_main.py`:
- Around line 35-43: The except ImportError block is too broad and catches all
ImportError exceptions, including those raised inside the imported module, which
silently hides real breakage. Restructure the try-except to only wrap the
__import__(module_name) call on line 36, moving the getattr, cls assignment, and
subsequent operations outside the try-except block so that ImportError
exceptions raised within the imported module are not silently caught and the
function fails fast instead of incorrectly falling back to another user class.
In `@projects/agentic_tools/locust/locust_users/_common.py`:
- Around line 77-80: The failure message on line 80 calls `data.keys()` without
verifying that `data` is actually a dictionary first. If the server returns a
JSON list or string with HTTP 200 status, this will raise an AttributeError. Add
a type check to ensure `data` is a dictionary before calling `.keys()` on it. If
`data` is not a dictionary, use a representation of the actual data type and
value in the failure message instead of trying to access `.keys()`. This ensures
the explicit failure handling path is executed properly regardless of the JSON
response type.
- Around line 30-33: The JSONL parsing loop in the file loading section lacks
error handling for json.loads() and the dictionary key access to "prompt". Wrap
the line containing json.loads(line)["prompt"] in a try-except block that
catches both JSONDecodeError and KeyError exceptions. When either exception
occurs, log a warning about the problematic line and continue to the next line
instead of crashing. This ensures one malformed JSONL entry does not abort the
entire prompt loading process during user startup.
In `@projects/agentic_tools/locust/locust_users/mcp_session_user.py`:
- Around line 28-29: The environment variable parsing for CALLS_PER_SESSION and
NUM_SERVERS using int() conversion can raise a ValueError if the environment
variables contain non-integer values, causing worker startup failures. Wrap both
int(os.environ.get()) calls in try-except blocks to catch ValueError exceptions,
and ensure each falls back to the default value (0) when parsing fails, allowing
the module to import successfully even with malformed environment inputs.
In `@projects/agentic_tools/locust/templates/locust_job.yaml`:
- Around line 29-33: Add a securityContext field to both container definitions
in the locust job template to enforce explicit security constraints. For the
locust container at line 29-33 (master pod) and the corresponding worker
container at lines 81-83, add a securityContext block that disables privilege
escalation, runs as a non-root user, and drops unnecessary capabilities to
comply with security best practices and prevent unintended privilege elevation.
In `@projects/agentic_tools/locust/toolbox/export_to_mlflow/main.py`:
- Around line 107-122: The function _inject_summary_into_mlflow_config is
selecting the latest summary.json file globally based on modification time using
sorted() and st_mtime, which can cause incorrect summaries to be injected into
the wrong MLflow run when multiple summary files exist. Instead of relying on
this mtime-based global selection, modify the function signature to accept an
explicit summary path parameter (or job_name identifier) from the caller, and
use that specific summary file rather than searching through the entire results
directory. This ensures each run injects only its own metrics into MLflow.
In `@projects/agentic_tools/locust/toolbox/generate_prompts/main.py`:
- Around line 57-85: The ensure_rbac function binds the prompt-generator-role to
the default ServiceAccount, which grants ConfigMap creation and deletion
permissions to all pods using that account in the namespace. Create a dedicated
ServiceAccount object in the rbac_manifest (with a name like
prompt-generator-sa) and update the RoleBinding's subjects section to reference
this new ServiceAccount by name instead of the default ServiceAccount to limit
permissions to only the intended job.
- Around line 210-214: The code accesses lines[0] without checking if the lines
list is empty, which will raise an IndexError and mask the underlying generation
failure. Add a guard check after the list comprehension that creates the lines
variable to verify that lines is not empty; if it is empty, raise a clear
exception with an informative error message indicating that no prompts were
generated instead of allowing the IndexError to occur when accessing first =
json.loads(lines[0]).
- Line 314: The AutoTokenizer.from_pretrained call with trust_remote_code=True
poses a security risk by allowing arbitrary code execution from untrusted
HuggingFace models, especially since TOKENIZER_MODEL is user-supplied without
validation. Change the trust_remote_code parameter from True to False in the
AutoTokenizer.from_pretrained call, as most tokenizers do not require remote
code execution. If any specific models genuinely require trust_remote_code=True,
add explicit validation to ensure only whitelisted trusted models are allowed to
use this setting.
In `@projects/agentic_tools/locust/toolbox/parse_results/main.py`:
- Around line 20-35: The _safe_float and _safe_int functions can still raise
ValueError when converting malformed string values to numeric types, even after
the None and "N/A" checks. Wrap the float(s) conversion in _safe_float (line 26)
and the int(float(s)) conversion in _safe_int (line 35) in try-except blocks
that catch ValueError exceptions and return the default value instead of
allowing the exception to propagate. This ensures the parsing pipeline continues
processing other rows and cells rather than failing entirely on a single
malformed value.
In `@projects/agentic_tools/locust/toolbox/run_distributed/main.py`:
- Around line 230-233: The code is silently accepting empty results from
_extract_section calls for CSV markers without validating that the expected data
was actually found. Add validation after the three _extract_section calls that
extract stats_csv, stats_history_csv, and failures_csv to check if any of these
return empty strings, and raise an appropriate error (such as ValueError) with a
descriptive message indicating which CSV marker was missing or malformed. This
same validation pattern should be applied to all similar _extract_section calls
throughout the function, including those around lines 245-252, to prevent
downstream code from treating broken master logs as valid data with empty
metrics.
- Around line 66-80: The run function does not guarantee cleanup when exceptions
occur during _wait_for_completion or _collect_results calls, leaving test
Jobs/Service artifacts behind. Restructure the run function to use a try-finally
block where the core logic (_create_configmap, _deploy_locust_job,
_wait_for_completion, _collect_results, and _save_artifacts if applicable) is in
the try block, and the cleanup logic (the if cleanup block with cleanup_job
call) is moved to the finally block to ensure cleanup always executes regardless
of whether an exception is raised.
- Around line 97-100: The code on the line `filepath = config.locustfiles_dir /
filename` assumes config.locustfiles_dir is set, but since the dataclass default
is None, this causes an opaque TypeError when attempting the path join
operation. Add a validation check before the loop that iterates over
config.locustfile_names to ensure config.locustfiles_dir is not None. If it is
None, raise a descriptive error explaining that locustfiles_dir must be
configured, allowing users to understand the configuration requirement rather
than encountering a cryptic TypeError.
In `@projects/agentic_tools/mcp/clients/mcp_client.py`:
- Around line 46-47: The timeout parameter with a default value of None allows
indefinite waits on transport stalls, which can freeze Locust users and impact
metrics. Replace the default value of None with a bounded timeout value (e.g., a
reasonable float like 30.0 or similar) for the timeout parameter in the
MCPClient class and all other timeout parameter definitions referenced at lines
135-140 and 166-171 to ensure all MCP HTTP calls have a maximum wait time.
In `@projects/agentic_tools/mcp/toolbox/deploy_mock_servers/main.py`:
- Around line 86-100: The readiness check is counting all deployments with the
SCALE_OUT_LABEL that have 1 ready replica, but it should validate readiness
specifically against the requested deployment names to prevent stale or
unrelated deployments from being counted. Modify the jsonpath query in the oc
call to filter deployments by the specific names that were requested/created
(not just by the label), or capture the deployment names separately and validate
that each of the requested deployments is actually ready by checking against the
returned names. This ensures that only the newly created servers are verified as
ready, not any unrelated deployments with the same label.
In `@projects/agentic_tools/mcp/toolbox/deploy_tokenized_mcp_server/main.py`:
- Around line 48-49: The rollout_timeout parameter is accepted in the run
function signature but is not passed to or used by any downstream functions in
the readiness flow. Wire the rollout_timeout parameter through the function
calls in the ranges around lines 63-66 and 185-215, ensuring it is passed to
whichever function handles the actual timeout logic during the readiness check,
so that the timeout behavior becomes configurable as documented in the public
contract.
In `@projects/caliper/prometheus_metrics/capture.py`:
- Around line 24-38: The subprocess.run() calls with timeout parameters can
raise subprocess.TimeoutExpired and OSError exceptions that are not currently
being caught, causing the metrics capture to abort instead of gracefully
handling failures. Wrap the subprocess.run() call (and the similar calls at
lines 46-58 and 74-95) in try-except blocks to catch both
subprocess.TimeoutExpired and OSError exceptions, and handle them gracefully by
logging the error and continuing execution rather than letting the exception
propagate and abort the metrics capture process.
- Around line 76-82: The curl command that includes the bearer token in the
Authorization header is currently using the -k flag which disables TLS
certificate validation, creating a security vulnerability where the token could
be exposed. Replace the -k flag in the curl command arguments with proper
certificate verification by removing -sk and replacing it with -s --cacert
/var/run/secrets/kubernetes.io/serviceaccount/ca.crt to use OpenShift's mounted
cluster CA certificate for secure server certificate validation while preserving
the bearer token's confidentiality.
In `@projects/core/dsl/utils/k8s.py`:
- Around line 295-301: The best_effort_oc function currently ignores non-zero
exit codes from the oc command because check=False prevents subprocess from
raising exceptions for failed commands. Capture the return value from the oc()
call and check its returncode attribute to detect when the command fails. If the
returncode is non-zero, log a warning message that includes the label and the
exit code so that failed cleanup operations are properly recorded instead of
silently disappearing.
In `@projects/mcp_gateway/orchestration/cleanup_phase.py`:
- Around line 65-97: The cleanup_platform_mod.run() and cleanup_platform_clone()
calls can fail and cause an exception that skips the namespace deletion logic.
Wrap these two function calls in a try-except block (with appropriate error
logging) so that the namespace teardown code starting with the
oc_resource_exists("namespace", namespace) check will always execute, ensuring
the test namespace and its resources are properly cleaned up even when platform
cleanup fails.
In `@projects/mcp_gateway/orchestration/config.d/infrastructure.yaml`:
- Around line 16-23: Replace the mutable image tag and null version references
in the infrastructure.yaml configuration with explicit, immutable versions to
ensure reproducible deployments. Update the catalog_image field from
"ghcr.io/kuadrant/mcp-controller-catalog:main" by replacing the `:main` tag with
a specific version tag or image digest. Additionally, replace the `version:
null` property in the mcp_gateway_instance section with an explicit chart
version number (such as "0.7.0"). These changes ensure that infrastructure
deployments remain consistent and do not silently drift across executions.
In `@projects/mcp_gateway/orchestration/config.d/mock_servers.yaml`:
- Line 2: The image specification in mock_servers.yaml uses the mutable `latest`
tag for the perf-mock-server image, which can cause inconsistent benchmark
results as the image may change between runs. Replace the `latest` tag with a
specific, immutable version tag (e.g., v1.0.0 or similar) and include the full
image digest (SHA256 hash) to ensure consistent, reproducible benchmark runs.
Update the image line to reference a concrete version and its digest in the
format quay.io/rh-ee-aharush/perf-mock-server:VERSION@sha256:HASH.
In `@projects/mcp_gateway/orchestration/test_phase.py`:
- Around line 63-119: The cleanup operations (cleanup_locust_job,
_capture_pod_logs, and _cleanup_servers) can be skipped if exceptions occur
during test execution or post-processing. Wrap the inner loop (iterating over
targets) that contains _run_test, parse_stats_csv, generate_summary,
capture_metrics, and export_to_mlflow calls in a try-finally block to ensure
cleanup_locust_job and _capture_pod_logs always execute. Similarly, wrap the
middle loop (iterating over concurrency) in a try-finally block to ensure
_cleanup_servers always executes, even if exceptions occur during the target
iteration. This guarantees that per-test and per-server cleanup happens
regardless of failures.
In `@projects/mcp_gateway/toolbox/cleanup_platform/main.py`:
- Around line 172-191: The cleanup logic unconditionally deletes shared system
namespaces (istio-system, istio-cni, kuadrant-system) if they exist, which is
destructive on non-ephemeral/shared clusters. Modify the namespaces list to only
include the shared system namespaces conditionally, based on whether the cluster
is ephemeral (likely through a flag in ctx or an environment variable). Separate
the context-specific namespaces (ctx.gateway_namespace,
ctx.mcp_gateway_namespace) which should always be deleted from the system
namespaces, and only add the system namespaces to the to_delete list if the
cluster is confirmed to be ephemeral before calling best_effort_oc.
In `@projects/mcp_gateway/toolbox/cleanup_test_resources/main.py`:
- Around line 91-120: The cleanup_forge_labeled() function is missing
"deployment" and "service" from its list of kinds to delete, which leaves
scale-out mock server workloads behind when cleaning up forge-labeled resources.
Add "deployment" and "service" to the kinds tuple in the for loop of the
cleanup_forge_labeled() function so that labeled Deployments and Services are
properly removed along with the jobs, pods, and configmaps.
In `@projects/mcp_gateway/toolbox/platform_helpers.py`:
- Around line 49-56: The function returns the existing directory at result_path
when result_path.is_dir() is true without validating that the cloned version
matches the currently requested version parameter. Modify the check at line 49
to not only verify that result_path exists as a directory, but also verify that
the existing clone is at the correct git ref for the requested version (resolve
the git ref for the requested version and compare it against what's currently
checked out). If the versions do not match, proceed with re-cloning at the
correct version instead of reusing the stale clone.
---
Minor comments:
In `@projects/agentic_tools/utils/token_text.py`:
- Around line 56-67: Add input validation at the start of the build_exact_text
function to check that max_attempts is greater than zero and valid_ids is not
empty; if either condition fails, raise an appropriate error. Additionally,
initialize the text variable before the for loop over max_attempts to ensure it
is always defined when the function returns, preventing an UnboundLocalError if
the loop never executes or the conditions are not met.
In `@projects/core/dsl/utils/k8s.py`:
- Around line 279-286: The condition on line 279 in the log writing logic
incorrectly treats successful commands with empty output as failures. The
condition currently checks both returncode == 0 AND log_result.stdout being
non-empty, but it should only check if returncode == 0 to consider a pod's empty
logs as successful. Modify the if statement to remove the "and
log_result.stdout" check so that any command with a successful exit code
(returncode == 0) writes the stdout to the log file, regardless of whether the
output is empty or not.
In `@projects/mcp_gateway/orchestration/runtime_config.py`:
- Around line 41-43: The get_spawn_rate method uses a truthiness check on the
raw value which incorrectly treats 0 as falsy and converts it to None, losing
the explicit spawn_rate=0 configuration. Replace the truthiness check with an
explicit check for None (e.g., if raw is not None) to preserve 0 as a valid
spawn_rate value instead of silently dropping it to None.
In `@projects/mcp_gateway/README.md`:
- Around line 7-41: The fenced code block starting on line 7 of the README.md
file is missing a language identifier, which triggers an MD040 linting warning.
Add the language identifier `text` immediately after the opening triple
backticks (```) to properly declare the code block format and resolve the lint
warning.
- Around line 82-86: The scale-out usage example in the README incorrectly
states that MCP_GATEWAY_VERSION is not needed, but the prepare_phase.run()
function in prepare_phase.py requires this environment variable to be set and
will fail without it. Update the scale-out example section in the README to
either clarify that MCP_GATEWAY_VERSION is required, or add an export statement
for MCP_GATEWAY_VERSION before the prepare, test, and post-cleanup command calls
to match the actual requirements of the prepare_phase.run() function.
In `@projects/mcp_gateway/toolbox/platform_helpers.py`:
- Around line 246-258: The temporary file created with NamedTemporaryFile(...,
delete=False) and stored in tmp_path is never cleaned up after being used in the
oc() function call, causing temp files to accumulate on disk over repeated runs.
After the oc() call completes (following the logger.info statement), explicitly
remove the temporary file using os.remove(tmp_path). Wrap this cleanup in a
try/finally block or similar error handling to ensure the file is deleted even
if the oc() call raises an exception.
---
Nitpick comments:
In `@projects/agentic_tools/locust/toolbox/generate_prompts/main.py`:
- Line 271: In the subprocess.check_call() command that installs the
transformers and kubernetes packages, replace the package names with
version-pinned specifications (e.g., transformers==VERSION and
kubernetes==VERSION) instead of using floating versions. This ensures
reproducible benchmark runs by guaranteeing the same package versions are
installed each time the script executes, preventing nondeterministic behavior
and unexpected breakage from upstream dependency changes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ebeab3c8-b59a-4ebc-a906-815c7ee9a242
📒 Files selected for processing (60)
.gitignoreprojects/agentic_tools/__init__.pyprojects/agentic_tools/base_runtime_config.pyprojects/agentic_tools/ci_base.pyprojects/agentic_tools/locust/__init__.pyprojects/agentic_tools/locust/locust_runtime/__init__.pyprojects/agentic_tools/locust/locust_runtime/locust_shapes.pyprojects/agentic_tools/locust/locust_runtime/locustfile_main.pyprojects/agentic_tools/locust/locust_runtime/metrics_hook.pyprojects/agentic_tools/locust/locust_users/__init__.pyprojects/agentic_tools/locust/locust_users/_common.pyprojects/agentic_tools/locust/locust_users/chat_completions_user.pyprojects/agentic_tools/locust/locust_users/mcp_session_user.pyprojects/agentic_tools/locust/locust_users/responses_mcp_benchmark_user.pyprojects/agentic_tools/locust/locust_users/responses_mcp_user.pyprojects/agentic_tools/locust/locust_users/responses_simple_user.pyprojects/agentic_tools/locust/locust_users/responses_users.pyprojects/agentic_tools/locust/templates/locust_job.yamlprojects/agentic_tools/locust/toolbox/__init__.pyprojects/agentic_tools/locust/toolbox/export_to_mlflow/__init__.pyprojects/agentic_tools/locust/toolbox/export_to_mlflow/main.pyprojects/agentic_tools/locust/toolbox/generate_prompts/__init__.pyprojects/agentic_tools/locust/toolbox/generate_prompts/main.pyprojects/agentic_tools/locust/toolbox/parse_results/main.pyprojects/agentic_tools/locust/toolbox/run_distributed/main.pyprojects/agentic_tools/mcp/__init__.pyprojects/agentic_tools/mcp/clients/__init__.pyprojects/agentic_tools/mcp/clients/mcp_client.pyprojects/agentic_tools/mcp/toolbox/__init__.pyprojects/agentic_tools/mcp/toolbox/deploy_mock_servers/__init__.pyprojects/agentic_tools/mcp/toolbox/deploy_mock_servers/main.pyprojects/agentic_tools/mcp/toolbox/deploy_tokenized_mcp_server/__init__.pyprojects/agentic_tools/mcp/toolbox/deploy_tokenized_mcp_server/main.pyprojects/agentic_tools/utils/__init__.pyprojects/agentic_tools/utils/token_text.pyprojects/caliper/prometheus_metrics/__init__.pyprojects/caliper/prometheus_metrics/capture.pyprojects/caliper/prometheus_metrics/config.pyprojects/caliper/prometheus_metrics/queries.pyprojects/caliper/prometheus_metrics/queries.yamlprojects/core/dsl/utils/k8s.pyprojects/mcp_gateway/README.mdprojects/mcp_gateway/orchestration/ci.pyprojects/mcp_gateway/orchestration/cleanup_phase.pyprojects/mcp_gateway/orchestration/config.d/experiment.yamlprojects/mcp_gateway/orchestration/config.d/infrastructure.yamlprojects/mcp_gateway/orchestration/config.d/mock_servers.yamlprojects/mcp_gateway/orchestration/config.d/runtime.yamlprojects/mcp_gateway/orchestration/config.yamlprojects/mcp_gateway/orchestration/prepare_phase.pyprojects/mcp_gateway/orchestration/presets.d/presets.yamlprojects/mcp_gateway/orchestration/runtime_config.pyprojects/mcp_gateway/orchestration/test_phase.pyprojects/mcp_gateway/toolbox/__init__.pyprojects/mcp_gateway/toolbox/apply_infrastructure/main.pyprojects/mcp_gateway/toolbox/cleanup_platform/main.pyprojects/mcp_gateway/toolbox/cleanup_test_resources/main.pyprojects/mcp_gateway/toolbox/install_platform/main.pyprojects/mcp_gateway/toolbox/platform_helpers.pypyproject.toml
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@projects/mcp_gateway/orchestration/pr_args.py`:
- Around line 85-90: The preset value extracted from args[0] is not validated
against the documented allowlist before being written to config_overrides. Add
validation logic to check that the preset value matches one of the allowed
options (smoke, baseline, scale-out, or demo) and raise an appropriate error if
it does not match any of these documented values. This validation should occur
immediately after extracting the preset from args[0] but before calling
config_overrides.update() to prevent invalid presets from reaching downstream
runtime configuration and test execution.
- Around line 97-103: The issue is that line.startswith("/version") matches
invalid commands like "/versionx" and the parser accepts extra tokens as part of
the version string. To fix this, replace the startswith check with strict
token-based parsing: split the line by whitespace, validate that the first token
is exactly "/version", ensure there is exactly one additional token (the version
string), and raise a ValueError if there are zero, two, or more tokens. This
ensures malformed directives are rejected at parse-time before they can be
written to config_overrides.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 51379d88-a377-41a2-9194-a3f062ceaa3f
📒 Files selected for processing (3)
projects/agentic_tools/mcp/toolbox/deploy_tokenized_mcp_server/main.pyprojects/mcp_gateway/orchestration/pr_args.pyprojects/mcp_gateway/orchestration/prepare_phase.py
🚧 Files skipped from review as they are similar to previous changes (2)
- projects/mcp_gateway/orchestration/prepare_phase.py
- projects/agentic_tools/mcp/toolbox/deploy_tokenized_mcp_server/main.py
|
/test fournos mcp_gateway smoke |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 00 minutes 23 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
|
/test fournos mcp_gateway smoke |
|
🔴 Test of 'mcp_gateway prepare' failed after 00 hours 01 minutes 08 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: |
|
🔴 Test of 'mcp_gateway export-artifacts' failed after 00 hours 00 minutes 01 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 02 minutes 26 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
|
/test fournos mcp_gateway smoke |
|
🔴 Test of 'mcp_gateway test' failed after 00 hours 00 minutes 00 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 03 minutes 56 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
| def get_preset_name() -> str: | ||
| args = config.project.get_config("project.args", []) | ||
| return args[0] if args else "default" |
There was a problem hiding this comment.
here you keep only the first parameter and swallow the rest. I would be cautious about that 🤔
maybe this?
return "|".join(args) if args else "default"
|
thanks @ashtarkb , great work 🎉 |
|
/approve |
1 similar comment
|
/approve |
Add three new FORGE projects for agentic AI performance testing: - projects/mcp_gateway: MCP Gateway load testing with Locust, including platform install/cleanup, mock server deployment, and MLflow export - projects/llamastack: LlamaStack deployment and benchmarking with vLLM inference, Postgres, and Locust-based load generation - projects/agentic_tools: Shared tooling (Locust runtime, MCP clients, mock server deployers, prompt generators, MLflow exporters) Supporting additions: - projects/core/library/base_runtime_config.py: reusable config base class - projects/core/library/ci_base.py: CI app factory to reduce repetitive setup - projects/core/dsl/utils/k8s.py: capture_pod_logs and best_effort_oc helpers - projects/caliper/metrics/: Prometheus metrics capture and plotting - pyproject.toml: add metrics optional dependency group - .gitignore: add vault secrets and credential patterns Co-authored-by: Cursor <cursoragent@cursor.com>
- Remove llamastack project (will be contributed separately) - Move ci_base.py and base_runtime_config.py from core to agentic_tools - Refactor ci_base from factory function to extensible CIApp class with overridable hooks (init, init_vaults, register_extra_commands) - Add PhaseSpec dataclass for type-safe phase definitions - Add hardware_resolver_func support and caliper_replot_entrypoint - Remove preset/config handling from ci_base (already done by config.init) - Rename caliper/metrics to caliper/prometheus_metrics - Make metrics YAML-driven: queries defined in queries.yaml (33 queries) - Remove plot.py (only raw JSON output needed) - Default to all queries when no subset specified - Add timestamp to MLflow run names for uniqueness Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Add projects/mcp_gateway/orchestration/pr_args.py to support: - /version directive for setting MCP Gateway version from PR comments - Preset selection from /test line (smoke, baseline, scale-out, demo) Update prepare_phase.py to read version from FORGE config (set by /version directive) with MCP_GATEWAY_VERSION env var fallback. Usage in PR comment: /test fournos mcp_gateway smoke /cluster agentic-cpt-8xa100 /pipeline forge-full /version 0.7.0 Co-authored-by: Cursor <cursoragent@cursor.com>
- Add psap-forge-mcp-gw-mlflow vault definition for project-specific MLflow credentials - Update config.yaml to use the new vault - Validate presets against allowlist (smoke, baseline, scale-out, demo) - Reject malformed /version directives (/versionx, multi-token values) Co-authored-by: Cursor <cursoragent@cursor.com>
…ault The forge-full pipeline image lacks helm, causing prepare phase to fail. Add _ensure_helm() to download it on demand (mirrors the oc auto-download pattern). Also fix caliper export to reference the project-specific psap-forge-mcp-gw-mlflow vault instead of the shared psap-forge-mlflow-export. Co-authored-by: Cursor <cursoragent@cursor.com>
…helm Read the version directly from the mcp-gateway deployment container image (e.g. ghcr.io/kuadrant/mcp-gateway:v0.7.0) via oc. Removes the helm dependency from the test phase since each Tekton step runs in a separate pod where helm is not available. Co-authored-by: Cursor <cursoragent@cursor.com>
…workspace Toolbox refactoring: - Rewrite run_distributed with @entrypoint + @task pattern, plain kwargs - Rewrite apply_infrastructure with @entrypoint + @task + @Retry - Update runtime_config and test_phase callers for new APIs - Add log_stdout=False to oc logs to avoid cluttering run.log - Remove redundant cleanup_infrastructure (use cleanup_test_resources) - Move parse_results to helpers (not a toolbox), remove export_to_mlflow Caliper MLflow enhancements: - Add multi-run export with parent/child MLflow runs - Add workspace support (MLFLOW_WORKSPACE env var + UI links) - Add mlflow_config.py workspace key validation - Single run dir falls back to flat export (no unnecessary nesting) - Add test_multi_run_export.py tests (23 tests) Co-authored-by: Cursor <cursoragent@cursor.com>
…_mock_servers - Add MCP Gateway Caliper plugin (parser, KPI handler, visualization) - Refactor test_phase.py: use run_and_postprocess + NextArtifactDir + write_test_labels - Refactor deploy_mock_servers to use @entrypoint/@task/@Retry DSL - Update caliper export to discover runs via __test_labels__.yaml only - Remove MLFLOW_WORKSPACE save/restore, unify mlflow.start_run calls - Add /artifacts to MLflow UI run links - Upload all artifacts to all runs (parent + children) - Add plugin and export unit tests Co-authored-by: Cursor <cursoragent@cursor.com>
…leanup pr_args/runtime_config - Save generated YAML to src/ dir in deploy_mock_servers and apply_infrastructure - Add @Always capture_artifacts tasks for post-mortem diagnostics - Log metrics/params for single (non-nested) MLflow runs - Remove get_supported_mcp_gateway_directives, align preset handling with framework - Remove _as_bool helper and redundant type casts in runtime_config Co-authored-by: Cursor <cursoragent@cursor.com>
Setting ci_job.args: [] prevented the preset name from flowing through to the fjob args → project.args, which meant apply_presets_from_project_args() found nothing to apply and the test matrix fell back to defaults (1 test instead of N). Co-authored-by: Cursor <cursoragent@cursor.com>
- Remove runtime.default_preset and runtime.selected_preset (not standard framework fields). Preset name now derived from project.args. - Remove ci_job.args override from pr_args.py so presets flow through the framework naturally. - Switch MLflow vault from psap-forge-mcp-gw-mlflow to the shared psap-forge-mlflow-export vault. - Delete unused psap-forge-mcp-gw-mlflow vault definition. Co-authored-by: Cursor <cursoragent@cursor.com>
- get_preset_name() now joins all args with "|" for naming - Remove single-preset restriction from pr_args.py Co-authored-by: Cursor <cursoragent@cursor.com>
Head branch was pushed to by a user without write access
f3d2167 to
b27124e
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ashtarkb, kpouget The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Add three new FORGE projects for agentic AI performance testing:
Supporting additions:
Summary by CodeRabbit