Adding AIQ shallow research eval script for RAG datasets#283
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds a new ChangesAI-Q RAG Evaluation CLI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 9
🤖 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 `@scripts/eval/evaluate_aiq_rag.py`:
- Around line 849-850: The current skip path in run_pipeline() returns too
early, which stops create_eval_dict() and ties inference-data generation to
RAGAS scoring. Update run_pipeline() and the related scoring flow so
skip_evaluation only disables the scoring step, while the inference-data
generation path still runs; use a separate scoring-only branch/flag in the
evaluate_aiq_rag.py pipeline. Also, where judge credentials are missing, make
the missing-secret path in the RAGAS/judge setup warn and skip scoring
gracefully instead of crashing, using the existing run_pipeline(),
create_eval_dict(), and skip_evaluation logic as the main points to adjust.
- Around line 564-567: Reject duplicate corpus basenames before building the
upload payload in the file upload flow that populates handles and files. Since
the code uses os.path.basename(file_path) and downstream reconciliation also
matches on basenames, add a pre-upload check over file_paths to detect duplicate
basenames and fail fast with a clear error before any open(file_path, "rb") or
files.append(...) work happens. Apply the same guard anywhere the corpus is
prepared for upload so recursive inputs with matching leaf filenames cannot be
skipped or misidentified.
- Around line 754-760: Validate the loaded eval rows in load_eval_data before
any workers start: after json.load and the existing list check, verify each item
is a dict with non-empty question and answer fields, and fail fast with a clear
error if any row is malformed or the dataset is empty. Use load_eval_data as the
single entry point for this validation so downstream worker code and RAGAS
scoring never receive invalid train.json rows.
- Around line 1026-1061: Add CLI validation in the argument parsing flow so
invalid numeric values are rejected before they reach execution. In the
parser/entrypoint around the argparse setup, enforce positive integers for
`--thread`, `--batch-size`, `--timeout`, and `--ingestion-timeout` in
`evaluate_aiq_rag.py`, using the existing argument names and defaults as the
main touchpoints. Make `--thread` and `--batch-size` require values greater than
0, and prevent negative values for the timeout options so downstream uses like
`ThreadPoolExecutor` and ingestion batching cannot fail at runtime.
- Around line 893-907: The context-metric gate in the evaluation flow is too
permissive because has_contexts uses any(...) and turns on ContextRelevance and
ResponseGroundedness for the whole EvaluationDataset even when some samples have
empty generated_contexts. Update the logic around evaluate_aiq_rag.py’s dataset
construction to either require every sample to have retrieved contexts before
adding those metrics, or build a filtered subset containing only samples with
generated_contexts and run the context metrics on that subset. Keep
AnswerAccuracy on the full set, and ensure SingleTurnSample creation and the
metrics list stay aligned with the chosen subset.
- Around line 1105-1110: The dataset loop in evaluate_aiq_rag.py does not guard
against duplicate run labels, so different paths with the same basename can
collide on output_dir, collection_name, and all_summaries. Add validation in the
dataset-processing loop before creating outputs to detect repeated run_label
values (or collisions after deriving collection_name) and fail fast with a clear
error instead of proceeding. Use the existing args.dataset_paths iteration and
the run_label/output_dir/all_summaries logic as the place to enforce uniqueness.
- Around line 74-77: The default judge model in the evaluate_aiq_rag.py module
is set to a different model ID than the documented public default, so unset
environment users may resolve the wrong target. Update the _DEFAULT_JUDGE_MODEL
constant used by JUDGE_MODEL to match the documented public model ID, and keep
the RAG_EVAL_JUDGE_MODEL override behavior unchanged so the environment variable
still takes precedence.
In `@scripts/eval/README.md`:
- Around line 115-116: Clarify the flags table entry for --skip-evaluation in
README so it explicitly says it skips RAGAS scoring only, while inference/output
generation still runs. Update the description alongside --skip-ingestion in the
flags table to remove the ambiguous “inference-only” wording and make the
partial-run behavior clear to operators.
- Around line 53-60: The dataset layout section in README should explicitly
document the required per-row schema for train.json, not just that it is a UTF-8
JSON array. Update the description near evaluate_aiq_rag.py and
validate_dataset_roots() to state that each array item must be an object with
question and answer keys, and make clear that missing either key will cause
evaluation to fail later.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 5987f490-c2f7-48c5-a695-233275248d78
⛔ Files ignored due to path filters (1)
scripts/eval/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
scripts/eval/README.mdscripts/eval/evaluate_aiq_rag.pyscripts/eval/pyproject.toml
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**
⚙️ CodeRabbit configuration file
**:AI-Q Agent Guidance
Repository-global instructions for coding agents and for humans reviewing
agent-authored changes. These rules apply to every task in this repository.
Task-specific runbooks live in.agents/skills/— load the
relevant skill before starting a workflow it covers.Project overview
AI-Q is an NVIDIA AI Blueprint: an enterprise research agent built on the
NeMo Agent Toolkit (NAT). The deployed product is a research blueprint, not
a general skill runtime. New retrieval sources and tools are NAT functions;
agent behavior is driven by workflow YAML, Jinja2 prompts, and a data-source
registry — not by hard-coded logic.Primary boundaries:
- Backend Python package:
src/aiq_agent/.- Data-source and tool packages:
sources/(each is its own package).- Frontends and tooling:
frontends/(web UI infrontends/ui/, eval harnesses
infrontends/benchmarks/).- Configs, deployment, docs:
configs/,deploy/,docs/.Stay inside this repository. If your workspace also contains adjacent repos
(for example a sibling NeMo-Relay checkout), do not edit them as part of an AI-Q
change. Treatsources/*as independent packages: prefer the smallest change
scoped to the package you are touching.Repository structure
Path Purpose src/aiq_agent/Backend agent, FastAPI extensions, auth, observability, knowledge sources/Data-source / tool packages (e.g. tavily_web_search,google_scholar_paper_search)configs/Workflow YAML configs (e.g. config_cli_default.yml)frontends/ui/Next.js / React / TypeScript / Tailwind / KUI web UI frontends/benchmarks/Eval harnesses: freshqa,deepsearch_qa,deepresearch_benchdeploy/Docker Compose and Helm/Kubernetes assets; deploy/.envfor secretsdocs/source/...
Files:
scripts/eval/pyproject.tomlscripts/eval/README.mdscripts/eval/evaluate_aiq_rag.py
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run ruff check and ruff format validation for Python code changes
**/*.py: Python code must be linted and formatted with Ruff using line length 120, target Python 3.11, rule sets E,F,W,I,PL,UP, and isort force-single-line configuration
Never commit secrets, tokens, or environment-specific hostnames; use environment variables and SecretStr instead, resolving API keys at runtime
Never print or log secret values, including in tool output or error messages
Missing-secret paths must degrade gracefully (stub/skip), not crash or leak
Do not hand-reformat unrelated code when making changes; match the existing import and formatting style
Files:
scripts/eval/evaluate_aiq_rag.py
🪛 ast-grep (0.44.0)
scripts/eval/evaluate_aiq_rag.py
[warning] 471-471: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.get(f"{self.server_url}{path}", timeout=30)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
[warning] 646-651: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.post(
url,
json=body,
headers=headers,
timeout=self._workflow_request_timeout(),
)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
[warning] 716-716: Request-controlled URL passed to requests; validate against an allowlist to prevent SSRF.
Context: requests.get(url, timeout=self._workflow_request_timeout(), stream=True)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-requests)
[warning] 523-523: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(file_path, "rb")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 564-564: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(file_path, "rb")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 750-750: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(self._evaluation_data_path(), "w", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 754-754: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(self.eval_data_path, encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 814-814: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(output_path, "w", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 1194-1194: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(summary_path, "w", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 1198-1198: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(results_path, "w", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 1202-1202: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(metrics_path, "w", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 1207-1207: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(combined_path, "w", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[info] 716-716: no timeout was given on call to external resource
Context: requests.get(url, timeout=self._workflow_request_timeout(), stream=True)
Note: [CWE-1088] Synchronous Access of Remote Resource without Timeout.
(requests-timeout)
🔇 Additional comments (1)
scripts/eval/pyproject.toml (1)
1-17: LGTM!
| for file_path in file_paths: | ||
| handle = open(file_path, "rb") | ||
| handles.append(handle) | ||
| files.append(("files", (os.path.basename(file_path), handle, "application/octet-stream"))) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject duplicate corpus basenames before upload.
Uploads strip paths to os.path.basename(file_path), and ingestion reconciliation also compares only basenames. A recursive corpus containing a/doc.pdf and b/doc.pdf can skip or misidentify one document.
Proposed guard
def collect_files_to_upload(self, ingested_documents: list[str]) -> list[str]:
ingested = {os.path.basename(name) for name in ingested_documents}
+ seen_basenames: dict[str, str] = {}
files_to_upload: list[str] = []
for root, _, files in os.walk(self.dataset_path):
for filename in files:
+ file_path = os.path.join(root, filename)
+ if filename in seen_basenames:
+ raise ValueError(
+ f"Duplicate corpus filename {filename!r}: {seen_basenames[filename]} and {file_path}. "
+ "Use unique basenames because the knowledge API stores file_name without relative paths."
+ )
+ seen_basenames[filename] = file_path
if filename not in ingested:
- files_to_upload.append(os.path.join(root, filename))
+ files_to_upload.append(file_path)
return files_to_uploadAlso applies to: 596-603
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 564-564: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(file_path, "rb")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 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 `@scripts/eval/evaluate_aiq_rag.py` around lines 564 - 567, Reject duplicate
corpus basenames before building the upload payload in the file upload flow that
populates handles and files. Since the code uses os.path.basename(file_path) and
downstream reconciliation also matches on basenames, add a pre-upload check over
file_paths to detect duplicate basenames and fail fast with a clear error before
any open(file_path, "rb") or files.append(...) work happens. Apply the same
guard anywhere the corpus is prepared for upload so recursive inputs with
matching leaf filenames cannot be skipped or misidentified.
| def load_eval_data(self) -> list[dict[str, Any]]: | ||
| with open(self.eval_data_path, encoding="utf-8") as handle: | ||
| data = json.load(handle) | ||
| if not isinstance(data, list): | ||
| print("Error: train.json must be a JSON array of objects with question/answer fields.") | ||
| sys.exit(1) | ||
| return data |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate train.json row shape before launching workers.
Later code assumes each row is a dict with a non-empty question and answer; malformed or empty datasets currently fail as worker errors or RAGAS scoring failures after inference has started.
Proposed validation
if not isinstance(data, list):
print("Error: train.json must be a JSON array of objects with question/answer fields.")
sys.exit(1)
+ if not data:
+ print("Error: train.json must contain at least one evaluation row.")
+ sys.exit(1)
+ for index, row in enumerate(data):
+ if (
+ not isinstance(row, dict)
+ or not str(row.get("question", "")).strip()
+ or row.get("answer") in (None, "")
+ ):
+ print(f"Error: train.json row {index} must include non-empty question and answer fields.")
+ sys.exit(1)
return data📝 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.
| def load_eval_data(self) -> list[dict[str, Any]]: | |
| with open(self.eval_data_path, encoding="utf-8") as handle: | |
| data = json.load(handle) | |
| if not isinstance(data, list): | |
| print("Error: train.json must be a JSON array of objects with question/answer fields.") | |
| sys.exit(1) | |
| return data | |
| def load_eval_data(self) -> list[dict[str, Any]]: | |
| with open(self.eval_data_path, encoding="utf-8") as handle: | |
| data = json.load(handle) | |
| if not isinstance(data, list): | |
| print("Error: train.json must be a JSON array of objects with question/answer fields.") | |
| sys.exit(1) | |
| if not data: | |
| print("Error: train.json must contain at least one evaluation row.") | |
| sys.exit(1) | |
| for index, row in enumerate(data): | |
| if ( | |
| not isinstance(row, dict) | |
| or not str(row.get("question", "")).strip() | |
| or row.get("answer") in (None, "") | |
| ): | |
| print(f"Error: train.json row {index} must include non-empty question and answer fields.") | |
| sys.exit(1) | |
| return data |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 754-754: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(self.eval_data_path, encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 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 `@scripts/eval/evaluate_aiq_rag.py` around lines 754 - 760, Validate the loaded
eval rows in load_eval_data before any workers start: after json.load and the
existing list check, verify each item is a dict with non-empty question and
answer fields, and fail fast with a clear error if any row is malformed or the
dataset is empty. Use load_eval_data as the single entry point for this
validation so downstream worker code and RAGAS scoring never receive invalid
train.json rows.
| if self.skip_evaluation: | ||
| return None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Separate RAGAS scoring skips from inference-data generation.
--skip-evaluation is documented and suggested as skipping scoring, but run_pipeline() returns before create_eval_dict(). Users without NVIDIA_API_KEY either crash or skip the whole inference-data generation path. Introduce a scoring-only branch/flag and let missing judge credentials warn and skip scoring instead. As per coding guidelines, “Missing-secret paths must degrade gracefully (stub/skip), not crash or leak.”
Also applies to: 1033-1036, 1063-1067
🤖 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 `@scripts/eval/evaluate_aiq_rag.py` around lines 849 - 850, The current skip
path in run_pipeline() returns too early, which stops create_eval_dict() and
ties inference-data generation to RAGAS scoring. Update run_pipeline() and the
related scoring flow so skip_evaluation only disables the scoring step, while
the inference-data generation path still runs; use a separate scoring-only
branch/flag in the evaluate_aiq_rag.py pipeline. Also, where judge credentials
are missing, make the missing-secret path in the RAGAS/judge setup warn and skip
scoring gracefully instead of crashing, using the existing run_pipeline(),
create_eval_dict(), and skip_evaluation logic as the main points to adjust.
Source: Coding guidelines
| has_contexts = any(sample.get("generated_contexts") for sample in eval_data) | ||
| if has_contexts: | ||
| eval_dataset = EvaluationDataset( | ||
| [ | ||
| SingleTurnSample( | ||
| user_input=sample["question"], | ||
| reference=sample["answer"], | ||
| response=sample["generated_answer"], | ||
| reference_contexts=sample.get("contexts", []), | ||
| retrieved_contexts=sample["generated_contexts"], | ||
| ) | ||
| for sample in eval_data | ||
| ] | ||
| ) | ||
| metrics = [AnswerAccuracy(), ContextRelevance(), ResponseGroundedness()] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not enable context metrics for partially missing retrieved contexts.
any(...) enables context metrics for the entire dataset when only one sample has contexts; rows with empty generated_contexts violate the documented metric precondition and can produce failures or NaNs. Evaluate context metrics on the filtered subset, or require all samples to have contexts before adding those metrics.
Minimal safe gate
- has_contexts = any(sample.get("generated_contexts") for sample in eval_data)
+ has_contexts = bool(eval_data) and all(sample.get("generated_contexts") for sample in eval_data)📝 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.
| has_contexts = any(sample.get("generated_contexts") for sample in eval_data) | |
| if has_contexts: | |
| eval_dataset = EvaluationDataset( | |
| [ | |
| SingleTurnSample( | |
| user_input=sample["question"], | |
| reference=sample["answer"], | |
| response=sample["generated_answer"], | |
| reference_contexts=sample.get("contexts", []), | |
| retrieved_contexts=sample["generated_contexts"], | |
| ) | |
| for sample in eval_data | |
| ] | |
| ) | |
| metrics = [AnswerAccuracy(), ContextRelevance(), ResponseGroundedness()] | |
| has_contexts = bool(eval_data) and all(sample.get("generated_contexts") for sample in eval_data) | |
| if has_contexts: | |
| eval_dataset = EvaluationDataset( | |
| [ | |
| SingleTurnSample( | |
| user_input=sample["question"], | |
| reference=sample["answer"], | |
| response=sample["generated_answer"], | |
| reference_contexts=sample.get("contexts", []), | |
| retrieved_contexts=sample["generated_contexts"], | |
| ) | |
| for sample in eval_data | |
| ] | |
| ) | |
| metrics = [AnswerAccuracy(), ContextRelevance(), ResponseGroundedness()] |
🤖 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 `@scripts/eval/evaluate_aiq_rag.py` around lines 893 - 907, The context-metric
gate in the evaluation flow is too permissive because has_contexts uses any(...)
and turns on ContextRelevance and ResponseGroundedness for the whole
EvaluationDataset even when some samples have empty generated_contexts. Update
the logic around evaluate_aiq_rag.py’s dataset construction to either require
every sample to have retrieved contexts before adding those metrics, or build a
filtered subset containing only samples with generated_contexts and run the
context metrics on that subset. Keep AnswerAccuracy on the full set, and ensure
SingleTurnSample creation and the metrics list stay aligned with the chosen
subset.
| "--thread", | ||
| default=DEFAULT_MAX_WORKERS, | ||
| type=int, | ||
| help="Parallel shallow-research jobs (default: 1).", | ||
| ) | ||
| parser.add_argument("--output-dir", default="results", help="Directory for evaluation outputs.") | ||
| parser.add_argument("--batch-size", default=DEFAULT_BATCH_SIZE, type=int, help="Ingestion upload batch size.") | ||
| parser.add_argument("--collection", default=None, help="Collection name (default: dataset directory basename).") | ||
| parser.add_argument("--skip-ingestion", action="store_true", help="Skip corpus ingestion.") | ||
| parser.add_argument("--skip-evaluation", action="store_true", help="Skip RAGAS scoring.") | ||
| parser.add_argument("--force-ingestion", action="store_true", help="Delete and recreate the collection.") | ||
| parser.add_argument( | ||
| "--inference-mode", | ||
| choices=("async", "atif"), | ||
| default=DEFAULT_INFERENCE_MODE, | ||
| help=( | ||
| "Inference API: 'async' uses /v1/jobs/async/submit (default, faster polling); " | ||
| "'atif' uses POST /v1/workflow/atif (slower, full ATIF trajectory)." | ||
| ), | ||
| ) | ||
| parser.add_argument( | ||
| "--timeout", | ||
| default=DEFAULT_TIMEOUT, | ||
| type=int, | ||
| help=( | ||
| "Per-query timeout in seconds (default: 1800). For async jobs this is the poll deadline; " | ||
| "for ATIF it is the max gap between stream chunks. Use 0 for no read timeout on ATIF." | ||
| ), | ||
| ) | ||
| parser.add_argument( | ||
| "--ingestion-timeout", | ||
| default=DEFAULT_INGESTION_TIMEOUT, | ||
| type=int, | ||
| help=("Max seconds to wait per ingestion batch (default: 3600). Scales with batch size up to this limit."), | ||
| ) | ||
| args = parser.parse_args() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate numeric CLI bounds before use.
--thread 0 raises in ThreadPoolExecutor, --batch-size 0 divides by zero, and negative timeouts produce immediate or inconsistent timeout behavior.
Proposed validation
args = parser.parse_args()
+
+ if args.thread < 1:
+ parser.error("--thread must be >= 1")
+ if args.batch_size < 1:
+ parser.error("--batch-size must be >= 1")
+ if args.timeout < 0:
+ parser.error("--timeout must be >= 0")
+ if args.ingestion_timeout < 1:
+ parser.error("--ingestion-timeout must be >= 1")📝 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.
| "--thread", | |
| default=DEFAULT_MAX_WORKERS, | |
| type=int, | |
| help="Parallel shallow-research jobs (default: 1).", | |
| ) | |
| parser.add_argument("--output-dir", default="results", help="Directory for evaluation outputs.") | |
| parser.add_argument("--batch-size", default=DEFAULT_BATCH_SIZE, type=int, help="Ingestion upload batch size.") | |
| parser.add_argument("--collection", default=None, help="Collection name (default: dataset directory basename).") | |
| parser.add_argument("--skip-ingestion", action="store_true", help="Skip corpus ingestion.") | |
| parser.add_argument("--skip-evaluation", action="store_true", help="Skip RAGAS scoring.") | |
| parser.add_argument("--force-ingestion", action="store_true", help="Delete and recreate the collection.") | |
| parser.add_argument( | |
| "--inference-mode", | |
| choices=("async", "atif"), | |
| default=DEFAULT_INFERENCE_MODE, | |
| help=( | |
| "Inference API: 'async' uses /v1/jobs/async/submit (default, faster polling); " | |
| "'atif' uses POST /v1/workflow/atif (slower, full ATIF trajectory)." | |
| ), | |
| ) | |
| parser.add_argument( | |
| "--timeout", | |
| default=DEFAULT_TIMEOUT, | |
| type=int, | |
| help=( | |
| "Per-query timeout in seconds (default: 1800). For async jobs this is the poll deadline; " | |
| "for ATIF it is the max gap between stream chunks. Use 0 for no read timeout on ATIF." | |
| ), | |
| ) | |
| parser.add_argument( | |
| "--ingestion-timeout", | |
| default=DEFAULT_INGESTION_TIMEOUT, | |
| type=int, | |
| help=("Max seconds to wait per ingestion batch (default: 3600). Scales with batch size up to this limit."), | |
| ) | |
| args = parser.parse_args() | |
| "--thread", | |
| default=DEFAULT_MAX_WORKERS, | |
| type=int, | |
| help="Parallel shallow-research jobs (default: 1).", | |
| ) | |
| parser.add_argument("--output-dir", default="results", help="Directory for evaluation outputs.") | |
| parser.add_argument("--batch-size", default=DEFAULT_BATCH_SIZE, type=int, help="Ingestion upload batch size.") | |
| parser.add_argument("--collection", default=None, help="Collection name (default: dataset directory basename).") | |
| parser.add_argument("--skip-ingestion", action="store_true", help="Skip corpus ingestion.") | |
| parser.add_argument("--skip-evaluation", action="store_true", help="Skip RAGAS scoring.") | |
| parser.add_argument("--force-ingestion", action="store_true", help="Delete and recreate the collection.") | |
| parser.add_argument( | |
| "--inference-mode", | |
| choices=("async", "atif"), | |
| default=DEFAULT_INFERENCE_MODE, | |
| help=( | |
| "Inference API: 'async' uses /v1/jobs/async/submit (default, faster polling); " | |
| "'atif' uses POST /v1/workflow/atif (slower, full ATIF trajectory)." | |
| ), | |
| ) | |
| parser.add_argument( | |
| "--timeout", | |
| default=DEFAULT_TIMEOUT, | |
| type=int, | |
| help=( | |
| "Per-query timeout in seconds (default: 1800). For async jobs this is the poll deadline; " | |
| "for ATIF it is the max gap between stream chunks. Use 0 for no read timeout on ATIF." | |
| ), | |
| ) | |
| parser.add_argument( | |
| "--ingestion-timeout", | |
| default=DEFAULT_INGESTION_TIMEOUT, | |
| type=int, | |
| help=("Max seconds to wait per ingestion batch (default: 3600). Scales with batch size up to this limit."), | |
| ) | |
| args = parser.parse_args() | |
| if args.thread < 1: | |
| parser.error("--thread must be >= 1") | |
| if args.batch_size < 1: | |
| parser.error("--batch-size must be >= 1") | |
| if args.timeout < 0: | |
| parser.error("--timeout must be >= 0") | |
| if args.ingestion_timeout < 1: | |
| parser.error("--ingestion-timeout must be >= 1") |
🤖 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 `@scripts/eval/evaluate_aiq_rag.py` around lines 1026 - 1061, Add CLI
validation in the argument parsing flow so invalid numeric values are rejected
before they reach execution. In the parser/entrypoint around the argparse setup,
enforce positive integers for `--thread`, `--batch-size`, `--timeout`, and
`--ingestion-timeout` in `evaluate_aiq_rag.py`, using the existing argument
names and defaults as the main touchpoints. Make `--thread` and `--batch-size`
require values greater than 0, and prevent negative values for the timeout
options so downstream uses like `ThreadPoolExecutor` and ingestion batching
cannot fail at runtime.
| for dataset_root in args.dataset_paths: | ||
| dataset_root = os.path.abspath(dataset_root) | ||
| run_label = os.path.basename(dataset_root.rstrip(os.sep)) or "dataset" | ||
| collection_name = args.collection or run_label | ||
| output_dir = os.path.join(args.output_dir, run_label) | ||
| os.makedirs(output_dir, exist_ok=True) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject duplicate dataset run labels before writing outputs.
run_label is only the dataset basename, so two paths ending in the same directory name share output_dir, collection name, and all_summaries key; the second run can overwrite or mix results.
Proposed validation
validate_dataset_roots(list(args.dataset_paths))
+ run_labels = [os.path.basename(os.path.abspath(path).rstrip(os.sep)) or "dataset" for path in args.dataset_paths]
+ duplicate_labels = sorted({label for label in run_labels if run_labels.count(label) > 1})
+ if duplicate_labels:
+ parser.error(f"Dataset basenames must be unique for output isolation: {duplicate_labels}")
data_sources = _parse_data_sources(args.data_sources)📝 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.
| for dataset_root in args.dataset_paths: | |
| dataset_root = os.path.abspath(dataset_root) | |
| run_label = os.path.basename(dataset_root.rstrip(os.sep)) or "dataset" | |
| collection_name = args.collection or run_label | |
| output_dir = os.path.join(args.output_dir, run_label) | |
| os.makedirs(output_dir, exist_ok=True) | |
| validate_dataset_roots(list(args.dataset_paths)) | |
| run_labels = [os.path.basename(os.path.abspath(path).rstrip(os.sep)) or "dataset" for path in args.dataset_paths] | |
| duplicate_labels = sorted({label for label in run_labels if run_labels.count(label) > 1}) | |
| if duplicate_labels: | |
| parser.error(f"Dataset basenames must be unique for output isolation: {duplicate_labels}") | |
| for dataset_root in args.dataset_paths: | |
| dataset_root = os.path.abspath(dataset_root) | |
| run_label = os.path.basename(dataset_root.rstrip(os.sep)) or "dataset" | |
| collection_name = args.collection or run_label | |
| output_dir = os.path.join(args.output_dir, run_label) | |
| os.makedirs(output_dir, exist_ok=True) |
🤖 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 `@scripts/eval/evaluate_aiq_rag.py` around lines 1105 - 1110, The dataset loop
in evaluate_aiq_rag.py does not guard against duplicate run labels, so different
paths with the same basename can collide on output_dir, collection_name, and
all_summaries. Add validation in the dataset-processing loop before creating
outputs to detect repeated run_label values (or collisions after deriving
collection_name) and fail fast with a clear error instead of proceeding. Use the
existing args.dataset_paths iteration and the run_label/output_dir/all_summaries
logic as the place to enforce uniqueness.
| train.json ← required; eval questions and answers | ||
| ``` | ||
|
|
||
| `evaluate_aiq_rag.py` validates each root with `validate_dataset_roots()`: | ||
|
|
||
| - The path must be a directory. | ||
| - `corpus/` must exist (files are discovered recursively). | ||
| - `train.json` must be a UTF-8 JSON array. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document required train.json row keys explicitly (question, answer).
The README says train.json is required, but not the exact per-row key contract. The evaluator expects objects with question/answer; missing keys will fail later. Add this explicitly in the dataset layout section.
🤖 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 `@scripts/eval/README.md` around lines 53 - 60, The dataset layout section in
README should explicitly document the required per-row schema for train.json,
not just that it is a UTF-8 JSON array. Update the description near
evaluate_aiq_rag.py and validate_dataset_roots() to state that each array item
must be an object with question and answer keys, and make clear that missing
either key will cause evaluation to fail later.
| | `--skip-ingestion` / `--skip-evaluation` | Ingest-only or inference-only partial runs | | ||
| | `--force-ingestion` | Delete and recreate the collection before upload | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify --skip-evaluation semantics in the flags table.
“inference-only” is ambiguous here. --skip-evaluation skips RAGAS scoring; inference/output generation still run. Rewording this avoids operator confusion in partial runs.
🤖 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 `@scripts/eval/README.md` around lines 115 - 116, Clarify the flags table entry
for --skip-evaluation in README so it explicitly says it skips RAGAS scoring
only, while inference/output generation still runs. Update the description
alongside --skip-ingestion in the flags table to remove the ambiguous
“inference-only” wording and make the partial-run behavior clear to operators.
niyatisingal
force-pushed
the
aiq-shallow-research-eval-script-for-rag-datasets
branch
from
767ab33 to
c411aa3
Compare
| def run_query(row: dict[str, Any]) -> dict[str, Any] | None: | ||
| question = row.get("question", "") | ||
| eval_query = ( | ||
| "Answer using only the documents in the knowledge base. " |
There was a problem hiding this comment.
This is a mistake it will direct agent to answer only using the kb base even when web search is enabled.
| default=DEFAULT_AGENT_TYPE, | ||
| help=f"Async agent type (default: {DEFAULT_AGENT_TYPE}).", | ||
| ) | ||
| parser.add_argument( |
There was a problem hiding this comment.
Make DEFAULT_DATA_SOURCES sources as empty, so that if nothing is passed, agent decides it. This is more intuitive.
There was a problem hiding this comment.
Added code to pickup all configured data sources if not explicitly provided
niyatisingal
force-pushed
the
aiq-shallow-research-eval-script-for-rag-datasets
branch
from
c411aa3 to
c7be568
Compare
3 participants
Overview
Validation
git commit -sor an equivalent sign-off.Where should reviewers start?
Related Issues
Summary by CodeRabbit
New Features
Chores