fix(experimentalist): improve getting started guide and testbed - #1009
fix(experimentalist): improve getting started guide and testbed#1009gaiadilorenzo wants to merge 14 commits into
Conversation
Signed-off-by: Gaia Di Lorenzo <gaia.dilorenzo01@gmail.com>
Signed-off-by: Gaia Di Lorenzo <gaia.dilorenzo01@gmail.com>
Signed-off-by: Gaia Di Lorenzo <gaia.dilorenzo01@gmail.com>
Signed-off-by: Gaia Di Lorenzo <gaia.dilorenzo01@gmail.com>
Signed-off-by: Gaia Di Lorenzo <gaia.dilorenzo01@gmail.com>
Signed-off-by: Gaia Di Lorenzo <gaia.dilorenzo01@gmail.com>
Signed-off-by: Gaia Di Lorenzo <gaia.dilorenzo01@gmail.com>
📝 WalkthroughWalkthroughThe PR adds a ChangesTau3 Harbor evaluation and ingestion
Experimentalist integration
Sequence Diagram(s)sequenceDiagram
participant User
participant HarborAdapter
participant HarborJob
participant OTLPBackend
participant Analyst
User->>HarborAdapter: run tau3-airline-harbor
HarborAdapter->>HarborJob: build and run evaluation
HarborJob-->>HarborAdapter: trial traces and verifier rewards
HarborAdapter->>OTLPBackend: upload traces and rewards
User->>Analyst: analyze recorded evaluation
Analyst->>OTLPBackend: read recorded workspace data
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 7
🧹 Nitpick comments (3)
plugins/nemo-insights/testbed/README.md (1)
208-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the Harbor workflow to a HOW-TO page.
This README contains reference content. Lines 208-226 add an execution procedure. Keep the subject definition here and link to a separate HOW-TO page.
As per coding guidelines, “Each documentation page should fit ONE Diataxis quadrant; do not mix tutorials with reference tables or how-tos with architecture explanations; use cross-links instead.”
🤖 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 `@plugins/nemo-insights/testbed/README.md` around lines 208 - 226, Move the Harbor benchmark execution commands and procedural details from the README into a dedicated HOW-TO page, while keeping the subject definition in the README. Add a cross-link from the README to the new HOW-TO page, and preserve the existing Harbor workflow content there without mixing it into the reference documentation.Source: Coding guidelines
plugins/nemo-insights/testbed/adapters.py (2)
433-445: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winParse rewards independently of
task_name.
result["task_name"]raisesKeyErrorbefore the reward parsing runs. A result file withouttask_nametherefore drops valid rewards. Readtask_namewith.getso both fields resolve independently.♻️ Proposed refactor
try: result = json.loads(result_path.read_text(encoding="utf-8")) - test_case_id = str(result["task_name"]) + if (task_name := result.get("task_name")) is not None: + test_case_id = str(task_name) raw_rewards = ( {} if result.get("exception_info") else (result.get("verifier_result") or {}).get("rewards") or {} ) rewards = { str(name): float(value) for name, value in raw_rewards.items() if isinstance(value, int | float) and not isinstance(value, bool) } - except (json.JSONDecodeError, KeyError, TypeError, ValueError): + except (json.JSONDecodeError, AttributeError, TypeError, ValueError): pass🤖 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 `@plugins/nemo-insights/testbed/adapters.py` around lines 433 - 445, Update the result parsing block so extracting task_case_id from result["task_name"] cannot abort reward extraction when the field is absent; use result.get("task_name") while preserving the existing string conversion behavior. Keep raw_rewards and rewards parsing independent so valid rewards are still returned for result files without task_name.
446-449: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
output.mime_typemay not match the artifact.Line 447 picks the first
output.*artifact alphabetically. Line 383 in_export_harbor_trace_filesthen always labels ittext/markdown. Anoutput.jsonoroutput.txtartifact gets a wrong MIME type. Derive the type from the suffix, or restrict the glob tooutput.md.🤖 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 `@plugins/nemo-insights/testbed/adapters.py` around lines 446 - 449, Update the artifact selection in the output-reading flow near _export_harbor_trace_files so the selected output artifact’s MIME type matches its actual suffix. Either restrict discovery to output.md or derive the MIME type for each supported output.* extension before labeling it, while preserving the existing output content handling.
🤖 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 `@plugins/nemo-experimentalist/examples/tau3-nooa-agent/prepare-airline.sh`:
- Line 10: Normalize the caller-provided OUTPUT_ROOT to an absolute path before
the subshell changes directories, while preserving the existing default path
behavior. Update the initialization around OUTPUT_ROOT and ensure the download,
validation, and copy steps all use this normalized value consistently.
- Around line 70-95: Update the split validation and copy logic around the
existing-task validation loops in the preparation script. Scan every existing
child task directory, require task.toml and the complete expected task contents,
and reject incomplete entries instead of treating them as absent. For missing
tasks, copy the source into a temporary directory and atomically publish or
rename it to the final ${split_root}/${task_name} path, preventing nested copies
and partially published tasks.
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.py`:
- Around line 142-146: Update the docker executable lookup in
_docker_cleanup_command to call shutil.which without explicitly passing
os.defpath, allowing it to use the process PATH and its built-in fallback when
PATH is unset. Preserve the existing FileNotFoundError behavior when no
executable is found.
- Around line 185-214: Refactor the resource cleanup loop around resource_specs
into independent async tasks so container, network, and volume listing/removal
workflows run concurrently. Preserve each resource type’s existing filtering,
cleanup ordering, exception warnings, and skip behavior, while awaiting all
tasks together so cancellation cleanup is bounded by the slowest resource
workflow rather than their sum.
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/model_config.py`:
- Around line 21-32: Update the Gemini detection in _completion_client to
recognize both provider-prefixed names and bare model names such as
gemini-3.5-flash, while preserving the existing
cache_control_injection_points=[] behavior for all Gemini models and the default
client construction for other providers.
In `@plugins/nemo-insights/testbed/adapters.py`:
- Around line 475-519: Update _inject_harbor_root_attributes and
_inject_resource_attributes to build their existing-key sets with
item.get("key")/attribute.get("key") instead of direct indexing, so malformed
attribute entries do not raise KeyError and abort export. Preserve the current
deduplication and conditional injection behavior.
- Around line 386-412: Update the export loop around _find_harbor_root_span_id
and export_trace_request so root-span lookup and trace export are handled in a
try/except that records one error without aborting the file, while incrementing
sent after a successful trace export. Separate post_evaluator_results into its
own per-reward error handling, marking each reward successful independently so
later lines retry only failed rewards and evaluator failures do not affect the
trace sent count.
---
Nitpick comments:
In `@plugins/nemo-insights/testbed/adapters.py`:
- Around line 433-445: Update the result parsing block so extracting
task_case_id from result["task_name"] cannot abort reward extraction when the
field is absent; use result.get("task_name") while preserving the existing
string conversion behavior. Keep raw_rewards and rewards parsing independent so
valid rewards are still returned for result files without task_name.
- Around line 446-449: Update the artifact selection in the output-reading flow
near _export_harbor_trace_files so the selected output artifact’s MIME type
matches its actual suffix. Either restrict discovery to output.md or derive the
MIME type for each supported output.* extension before labeling it, while
preserving the existing output content handling.
In `@plugins/nemo-insights/testbed/README.md`:
- Around line 208-226: Move the Harbor benchmark execution commands and
procedural details from the README into a dedicated HOW-TO page, while keeping
the subject definition in the README. Add a cross-link from the README to the
new HOW-TO page, and preserve the existing Harbor workflow content there without
mixing it into the reference documentation.
🪄 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: CHILL
Plan: Enterprise
Run ID: b20e596a-6837-437c-8255-5c46dbc6667c
📒 Files selected for processing (18)
docs/get-started/example-agent.mdxplugins/nemo-experimentalist/benchmarks/configs/tau3-quality.yamlplugins/nemo-experimentalist/examples/tau3-nooa-agent/experimentalist-quality.yamlplugins/nemo-experimentalist/examples/tau3-nooa-agent/prepare-airline-smoke.shplugins/nemo-experimentalist/examples/tau3-nooa-agent/prepare-airline.shplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/model_config.pyplugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor.pyplugins/nemo-experimentalist/tests/experimentalist/test_model_config.pyplugins/nemo-insights/testbed/README.mdplugins/nemo-insights/testbed/adapters.pyplugins/nemo-insights/testbed/cli.pyplugins/nemo-insights/testbed/otlp_ingest.pyplugins/nemo-insights/testbed/testbeds.tomlplugins/nemo-insights/tests/testbed/test_adapters.pyplugins/nemo-insights/tests/testbed/test_cli.pyplugins/nemo-insights/tests/testbed/test_otlp_ingest.pyplugins/nemo-insights/tests/testbed/test_registry.py
💤 Files with no reviewable changes (2)
- plugins/nemo-experimentalist/examples/tau3-nooa-agent/prepare-airline-smoke.sh
- plugins/nemo-experimentalist/benchmarks/configs/tau3-quality.yaml
|
|
||
| SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" | ||
| PLUGIN_ROOT="$(cd -- "${SCRIPT_DIR}/../.." && pwd)" | ||
| OUTPUT_ROOT="${1:-${PLUGIN_ROOT}/tmp/tau3-airline-smoke}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Normalize OUTPUT_ROOT before changing directories.
When the caller passes a relative output path, lines 52-60 resolve it from the caller’s directory, but line 56 runs after cd "${PLUGIN_ROOT}". The download therefore uses a different directory than the later validation and copy steps. prepare-airline.sh tmp/out can fail or write data outside the requested output path.
Convert OUTPUT_ROOT to an absolute path before the subshell.
Proposed fix
OUTPUT_ROOT="${1:-${PLUGIN_ROOT}/tmp/tau3-airline-smoke}"
+if [[ "${OUTPUT_ROOT}" != /* ]]; then
+ OUTPUT_ROOT="${PWD}/${OUTPUT_ROOT}"
+fiAlso applies to: 52-60
🤖 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 `@plugins/nemo-experimentalist/examples/tau3-nooa-agent/prepare-airline.sh` at
line 10, Normalize the caller-provided OUTPUT_ROOT to an absolute path before
the subshell changes directories, while preserving the existing default path
behavior. Update the initialization around OUTPUT_ROOT and ensure the download,
validation, and copy steps all use this normalized value consistently.
| for existing_task_path in "${split_root}"/*/task.toml; do | ||
| existing_task_name="$(basename -- "$(dirname -- "${existing_task_path}")")" | ||
| expected=false | ||
| for task_name in "${task_names[@]}"; do | ||
| if [[ "${existing_task_name}" == "${task_name}" ]]; then | ||
| expected=true | ||
| break | ||
| fi | ||
| done | ||
| if [[ "${expected}" != true ]]; then | ||
| echo "Existing split contains unexpected task ${existing_task_name}: ${split_root}" >&2 | ||
| exit 1 | ||
| fi | ||
| done | ||
|
|
||
| for task_name in "${task_names[@]}"; do | ||
| if [[ -f "${split_root}/${task_name}/task.toml" ]]; then | ||
| continue | ||
| fi | ||
| local source_task="${DATASET_ROOT}/${task_name}" | ||
| if [[ ! -f "${source_task}/task.toml" ]]; then | ||
| echo "Downloaded dataset is missing ${task_name}" >&2 | ||
| exit 1 | ||
| fi | ||
| cp -R "${source_task}" "${split_root}/${task_name}" | ||
| done |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject incomplete task directories before copying.
The loop only detects child directories that already contain task.toml. If an interrupted copy leaves ${split_root}/${task_name} without that file, line 94 copies the source directory inside the existing directory, creating a nested task path. If task.toml exists but other files are missing, lines 86-88 skip the incomplete task. The script can then report a ready split with invalid task contents.
Scan every existing task directory, reject incomplete entries, and copy each new task into a temporary directory before publishing it.
🤖 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 `@plugins/nemo-experimentalist/examples/tau3-nooa-agent/prepare-airline.sh`
around lines 70 - 95, Update the split validation and copy logic around the
existing-task validation loops in the preparation script. Scan every existing
child task directory, require task.toml and the complete expected task contents,
and reject incomplete entries instead of treating them as absent. For missing
tasks, copy the source into a temporary directory and atomically publish or
rename it to the final ${split_root}/${task_name} path, preventing nested copies
and partially published tasks.
| async def _docker_cleanup_command(args: Sequence[str]) -> str: | ||
| docker_path = shutil.which("docker", path=os.defpath) | ||
| if docker_path is None: | ||
| raise FileNotFoundError("docker executable not found on the system path") | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fix shutil.which path restriction.
shutil.which("docker", path=os.defpath) ignores the real PATH environment variable. By default, shutil.which uses PATH from os.environ and only falls back to os.defpath when PATH is unset.When no path is specified, the PATH environment variable is read from os.environ, falling back to os.defpath if it is not set. Passing path=os.defpath explicitly forces the search into a small, fixed set of directories (commonly just /bin:/usr/bin), skipping Homebrew, Docker Desktop, snap, and other common Docker install locations entirely.
Because every failure here is caught and only logged as a warning in _cleanup_cancelled_harbor_projects (Lines 199-201, 213-214), this can make the whole cancellation-cleanup feature silently do nothing on many developer and CI machines where docker is not in those two directories.
🔧 Proposed fix
- docker_path = shutil.which("docker", path=os.defpath)
+ docker_path = shutil.which("docker")📝 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.
| async def _docker_cleanup_command(args: Sequence[str]) -> str: | |
| docker_path = shutil.which("docker", path=os.defpath) | |
| if docker_path is None: | |
| raise FileNotFoundError("docker executable not found on the system path") | |
| async def _docker_cleanup_command(args: Sequence[str]) -> str: | |
| docker_path = shutil.which("docker") | |
| if docker_path is None: | |
| raise FileNotFoundError("docker executable not found on the system path") |
🤖 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
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.py`
around lines 142 - 146, Update the docker executable lookup in
_docker_cleanup_command to call shutil.which without explicitly passing
os.defpath, allowing it to use the process PATH and its built-in fallback when
PATH is unset. Preserve the existing FileNotFoundError behavior when no
executable is found.
| for resource, id_template, remove_command in resource_specs: | ||
| list_command = [resource, "ls"] | ||
| if resource == "container": | ||
| list_command.append("--all") | ||
| list_command.extend( | ||
| ( | ||
| "--filter", | ||
| f"label={_DOCKER_COMPOSE_PROJECT_LABEL}", | ||
| "--format", | ||
| f'{{{{{id_template}}}}}\t{{{{.Label "{_DOCKER_COMPOSE_PROJECT_LABEL}"}}}}', | ||
| ) | ||
| ) | ||
| try: | ||
| rows = await _docker_cleanup_command(list_command) | ||
| except Exception as exc: | ||
| logger.warning("Could not list Harbor %s resources during cancellation cleanup: %s", resource, exc) | ||
| continue | ||
|
|
||
| resource_ids = [] | ||
| for row in rows.splitlines(): | ||
| resource_id, separator, project = row.partition("\t") | ||
| if separator and any(project.startswith(prefix) for prefix in project_prefixes): | ||
| resource_ids.append(resource_id) | ||
| if not resource_ids: | ||
| continue | ||
|
|
||
| try: | ||
| await _docker_cleanup_command((*remove_command, *resource_ids)) | ||
| except Exception as exc: | ||
| logger.warning("Could not remove cancelled Harbor %s resources: %s", resource, exc) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Sequential per-resource-type cleanup can delay cancellation exit substantially.
The container, network, and volume loops run sequentially. Each type issues up to two Docker calls (list, then remove) at _DOCKER_CLEANUP_TIMEOUT_SEC = 30.0 each. In the worst case (a hung or slow Docker daemon), this totals up to 180 seconds. _run (Lines 1381-1382) awaits this work under asyncio.shield, so the process does not exit and does not respond to further cancellation until the full sequence finishes.
Run the three resource types concurrently to bound the worst case closer to one timeout window instead of six.
⚡ Proposed fix
- for resource, id_template, remove_command in resource_specs:
- list_command = [resource, "ls"]
- if resource == "container":
- list_command.append("--all")
- list_command.extend(
- (
- "--filter",
- f"label={_DOCKER_COMPOSE_PROJECT_LABEL}",
- "--format",
- f'{{{{{id_template}}}}}\t{{{{.Label "{_DOCKER_COMPOSE_PROJECT_LABEL}"}}}}',
- )
- )
- try:
- rows = await _docker_cleanup_command(list_command)
- except Exception as exc:
- logger.warning("Could not list Harbor %s resources during cancellation cleanup: %s", resource, exc)
- continue
-
- resource_ids = []
- for row in rows.splitlines():
- resource_id, separator, project = row.partition("\t")
- if separator and any(project.startswith(prefix) for prefix in project_prefixes):
- resource_ids.append(resource_id)
- if not resource_ids:
- continue
-
- try:
- await _docker_cleanup_command((*remove_command, *resource_ids))
- except Exception as exc:
- logger.warning("Could not remove cancelled Harbor %s resources: %s", resource, exc)
+ async def _cleanup_resource(resource: str, id_template: str, remove_command: tuple[str, ...]) -> None:
+ list_command = [resource, "ls"]
+ if resource == "container":
+ list_command.append("--all")
+ list_command.extend(
+ (
+ "--filter",
+ f"label={_DOCKER_COMPOSE_PROJECT_LABEL}",
+ "--format",
+ f'{{{{{id_template}}}}}\t{{{{.Label "{_DOCKER_COMPOSE_PROJECT_LABEL}"}}}}',
+ )
+ )
+ try:
+ rows = await _docker_cleanup_command(list_command)
+ except Exception as exc:
+ logger.warning("Could not list Harbor %s resources during cancellation cleanup: %s", resource, exc)
+ return
+
+ resource_ids = [
+ resource_id
+ for resource_id, separator, project in (row.partition("\t") for row in rows.splitlines())
+ if separator and any(project.startswith(prefix) for prefix in project_prefixes)
+ ]
+ if not resource_ids:
+ return
+
+ try:
+ await _docker_cleanup_command((*remove_command, *resource_ids))
+ except Exception as exc:
+ logger.warning("Could not remove cancelled Harbor %s resources: %s", resource, exc)
+
+ await asyncio.gather(*(_cleanup_resource(*spec) for spec in resource_specs))🤖 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
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.py`
around lines 185 - 214, Refactor the resource cleanup loop around resource_specs
into independent async tasks so container, network, and volume listing/removal
workflows run concurrently. Preserve each resource type’s existing filtering,
cleanup ordering, exception warnings, and skip behavior, while awaiting all
tasks together so cancellation cleanup is bounded by the slowest resource
workflow rather than their sum.
| def _completion_client(name: str, *, api_base: str, api_key: str) -> CompletionClient: | ||
| if "/gemini-" in name.lower(): | ||
| # NoOA injects explicit prompt-cache breakpoints by default. Vertex AI | ||
| # rejects those breakpoints when the cacheable prefix is below its | ||
| # minimum token count, so Gemini must use provider-managed caching. | ||
| return CompletionClient( | ||
| name, | ||
| api_base=api_base, | ||
| api_key=api_key, | ||
| cache_control_injection_points=[], | ||
| ) | ||
| return CompletionClient(name, api_base=api_base, api_key=api_key) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Gemini detection misses bare model names without a path prefix.
"/gemini-" in name.lower() requires a / before gemini-. A model name configured as gemini-3.5-flash (no provider path prefix) would not disable cache_control_injection_points, defeating the Vertex AI workaround this function exists for. All current defaults use a prefixed path, so this has a safe fallback today, but nothing prevents a differently-formatted EXPERIMENTALIST_*_MODEL_NAME value from silently reintroducing the rejected cache breakpoints.
🛡️ Proposed fix
- if "/gemini-" in name.lower():
+ if re.search(r"(^|/)gemini-", name.lower()):🤖 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
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/model_config.py`
around lines 21 - 32, Update the Gemini detection in _completion_client to
recognize both provider-prefixed names and bare model names such as
gemini-3.5-flash, while preserving the existing
cache_control_injection_points=[] behavior for all Gemini models and the default
client construction for other providers.
| root_span_id = _find_harbor_root_span_id(resource_spans) | ||
| try: | ||
| request = ParseDict(body, ExportTraceServiceRequest()) | ||
| export_trace_request( | ||
| base_url, | ||
| workspace, | ||
| request, | ||
| client=active_client, | ||
| headers=headers, | ||
| ) | ||
| if root_span_id is not None and not evaluator_posted: | ||
| for reward_name, reward_value in rewards.items(): | ||
| post_evaluator_results( | ||
| base_url, | ||
| workspace, | ||
| span_id=root_span_id, | ||
| session_id=session_id, | ||
| score=reward_value, | ||
| name=reward_name, | ||
| client=active_client, | ||
| ) | ||
| evaluator_posted = True | ||
| except Exception as exc: | ||
| print(f"Harbor trace {path.name} line {line_number}: export error — {exc}", file=sys.stderr) | ||
| errors += 1 | ||
| else: | ||
| sent += 1 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Separate the reward post from the trace export.
Both calls share one try. If post_evaluator_results raises for the second reward, evaluator_posted stays False and sent is not incremented, although the trace exported. The next line in the same file then re-posts every reward, including the one that already succeeded. This produces duplicate evaluator rows and an undercounted sent.
Also, _find_harbor_root_span_id at Line 386 runs outside the try. A span without spanId raises KeyError and aborts the whole export instead of counting one error.
🔧 Proposed fix
- root_span_id = _find_harbor_root_span_id(resource_spans)
try:
+ root_span_id = _find_harbor_root_span_id(resource_spans)
request = ParseDict(body, ExportTraceServiceRequest())
export_trace_request(
base_url,
workspace,
request,
client=active_client,
headers=headers,
)
- if root_span_id is not None and not evaluator_posted:
- for reward_name, reward_value in rewards.items():
- post_evaluator_results(
- base_url,
- workspace,
- span_id=root_span_id,
- session_id=session_id,
- score=reward_value,
- name=reward_name,
- client=active_client,
- )
- evaluator_posted = True
except Exception as exc:
print(f"Harbor trace {path.name} line {line_number}: export error — {exc}", file=sys.stderr)
errors += 1
else:
sent += 1
+ if root_span_id is not None and not evaluator_posted:
+ evaluator_posted = True
+ for reward_name, reward_value in rewards.items():
+ try:
+ post_evaluator_results(
+ base_url,
+ workspace,
+ span_id=root_span_id,
+ session_id=session_id,
+ score=reward_value,
+ name=reward_name,
+ client=active_client,
+ )
+ except Exception as exc:
+ print(
+ f"Harbor trace {path.name} line {line_number}: reward '{reward_name}' — {exc}",
+ file=sys.stderr,
+ )
+ errors += 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.
| root_span_id = _find_harbor_root_span_id(resource_spans) | |
| try: | |
| request = ParseDict(body, ExportTraceServiceRequest()) | |
| export_trace_request( | |
| base_url, | |
| workspace, | |
| request, | |
| client=active_client, | |
| headers=headers, | |
| ) | |
| if root_span_id is not None and not evaluator_posted: | |
| for reward_name, reward_value in rewards.items(): | |
| post_evaluator_results( | |
| base_url, | |
| workspace, | |
| span_id=root_span_id, | |
| session_id=session_id, | |
| score=reward_value, | |
| name=reward_name, | |
| client=active_client, | |
| ) | |
| evaluator_posted = True | |
| except Exception as exc: | |
| print(f"Harbor trace {path.name} line {line_number}: export error — {exc}", file=sys.stderr) | |
| errors += 1 | |
| else: | |
| sent += 1 | |
| try: | |
| root_span_id = _find_harbor_root_span_id(resource_spans) | |
| request = ParseDict(body, ExportTraceServiceRequest()) | |
| export_trace_request( | |
| base_url, | |
| workspace, | |
| request, | |
| client=active_client, | |
| headers=headers, | |
| ) | |
| except Exception as exc: | |
| print(f"Harbor trace {path.name} line {line_number}: export error — {exc}", file=sys.stderr) | |
| errors += 1 | |
| else: | |
| sent += 1 | |
| if root_span_id is not None and not evaluator_posted: | |
| evaluator_posted = True | |
| for reward_name, reward_value in rewards.items(): | |
| try: | |
| post_evaluator_results( | |
| base_url, | |
| workspace, | |
| span_id=root_span_id, | |
| session_id=session_id, | |
| score=reward_value, | |
| name=reward_name, | |
| client=active_client, | |
| ) | |
| except Exception as exc: | |
| print( | |
| f"Harbor trace {path.name} line {line_number}: reward '{reward_name}' — {exc}", | |
| file=sys.stderr, | |
| ) | |
| errors += 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 `@plugins/nemo-insights/testbed/adapters.py` around lines 386 - 412, Update the
export loop around _find_harbor_root_span_id and export_trace_request so
root-span lookup and trace export are handled in a try/except that records one
error without aborting the file, while incrementing sent after a successful
trace export. Separate post_evaluator_results into its own per-reward error
handling, marking each reward successful independently so later lines retry only
failed rewards and evaluator failures do not affect the trace sent count.
| def _inject_harbor_root_attributes(resource_spans: list[dict[str, Any]], extra: Mapping[str, str | None]) -> None: | ||
| for span in _harbor_root_spans(resource_spans): | ||
| attributes = span.setdefault("attributes", []) | ||
| existing = {item["key"] for item in attributes} | ||
| for key, value in extra.items(): | ||
| if value is not None and key not in existing: | ||
| attributes.append({"key": key, "value": {"stringValue": value}}) | ||
|
|
||
|
|
||
| def _find_harbor_root_span_id(resource_spans: list[dict[str, Any]]) -> str | None: | ||
| """Return root AGENT span ID in Intake's hexadecimal form.""" | ||
| root = _find_harbor_root_span(resource_spans) | ||
| if root is None: | ||
| return None | ||
| return base64.b64decode(root["spanId"]).hex() | ||
|
|
||
|
|
||
| def _find_harbor_root_span(resource_spans: list[dict[str, Any]]) -> dict[str, Any] | None: | ||
| return next(_harbor_root_spans(resource_spans), None) | ||
|
|
||
|
|
||
| def _harbor_root_spans(resource_spans: list[dict[str, Any]]) -> Iterator[dict[str, Any]]: | ||
| """Yield root AGENT spans from OTLP JSON.""" | ||
| for resource_span in resource_spans: | ||
| for scope_spans in resource_span.get("scopeSpans", []): | ||
| for span in scope_spans.get("spans", []): | ||
| kind = next( | ||
| ( | ||
| item.get("value", {}).get("stringValue") | ||
| for item in span.get("attributes", []) | ||
| if item.get("key") == "openinference.span.kind" | ||
| ), | ||
| None, | ||
| ) | ||
| if not span.get("parentSpanId") and kind == "AGENT": | ||
| yield span | ||
|
|
||
|
|
||
| def _inject_resource_attributes(resource_spans: list[dict[str, Any]], extra: Mapping[str, str | None]) -> None: | ||
| for resource_span in resource_spans: | ||
| attributes = resource_span.setdefault("resource", {}).setdefault("attributes", []) | ||
| existing = {attribute["key"] for attribute in attributes} | ||
| for key, value in extra.items(): | ||
| if value is not None and key not in existing: | ||
| attributes.append({"key": key, "value": {"stringValue": value}}) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use .get("key") in the injectors.
Lines 478 and 516 index item["key"] directly. Every other reader in this file uses .get("key"). An attribute entry without key raises KeyError and aborts the entire export loop, because these calls run outside the per-line try.
🛡️ Proposed fix
- existing = {item["key"] for item in attributes}
+ existing = {item.get("key") for item in attributes}- existing = {attribute["key"] for attribute in attributes}
+ existing = {attribute.get("key") for attribute in attributes}📝 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 _inject_harbor_root_attributes(resource_spans: list[dict[str, Any]], extra: Mapping[str, str | None]) -> None: | |
| for span in _harbor_root_spans(resource_spans): | |
| attributes = span.setdefault("attributes", []) | |
| existing = {item["key"] for item in attributes} | |
| for key, value in extra.items(): | |
| if value is not None and key not in existing: | |
| attributes.append({"key": key, "value": {"stringValue": value}}) | |
| def _find_harbor_root_span_id(resource_spans: list[dict[str, Any]]) -> str | None: | |
| """Return root AGENT span ID in Intake's hexadecimal form.""" | |
| root = _find_harbor_root_span(resource_spans) | |
| if root is None: | |
| return None | |
| return base64.b64decode(root["spanId"]).hex() | |
| def _find_harbor_root_span(resource_spans: list[dict[str, Any]]) -> dict[str, Any] | None: | |
| return next(_harbor_root_spans(resource_spans), None) | |
| def _harbor_root_spans(resource_spans: list[dict[str, Any]]) -> Iterator[dict[str, Any]]: | |
| """Yield root AGENT spans from OTLP JSON.""" | |
| for resource_span in resource_spans: | |
| for scope_spans in resource_span.get("scopeSpans", []): | |
| for span in scope_spans.get("spans", []): | |
| kind = next( | |
| ( | |
| item.get("value", {}).get("stringValue") | |
| for item in span.get("attributes", []) | |
| if item.get("key") == "openinference.span.kind" | |
| ), | |
| None, | |
| ) | |
| if not span.get("parentSpanId") and kind == "AGENT": | |
| yield span | |
| def _inject_resource_attributes(resource_spans: list[dict[str, Any]], extra: Mapping[str, str | None]) -> None: | |
| for resource_span in resource_spans: | |
| attributes = resource_span.setdefault("resource", {}).setdefault("attributes", []) | |
| existing = {attribute["key"] for attribute in attributes} | |
| for key, value in extra.items(): | |
| if value is not None and key not in existing: | |
| attributes.append({"key": key, "value": {"stringValue": value}}) | |
| def _inject_harbor_root_attributes(resource_spans: list[dict[str, Any]], extra: Mapping[str, str | None]) -> None: | |
| for span in _harbor_root_spans(resource_spans): | |
| attributes = span.setdefault("attributes", []) | |
| existing = {item.get("key") for item in attributes} | |
| for key, value in extra.items(): | |
| if value is not None and key not in existing: | |
| attributes.append({"key": key, "value": {"stringValue": value}}) | |
| def _find_harbor_root_span_id(resource_spans: list[dict[str, Any]]) -> str | None: | |
| """Return root AGENT span ID in Intake's hexadecimal form.""" | |
| root = _find_harbor_root_span(resource_spans) | |
| if root is None: | |
| return None | |
| return base64.b64decode(root["spanId"]).hex() | |
| def _find_harbor_root_span(resource_spans: list[dict[str, Any]]) -> dict[str, Any] | None: | |
| return next(_harbor_root_spans(resource_spans), None) | |
| def _harbor_root_spans(resource_spans: list[dict[str, Any]]) -> Iterator[dict[str, Any]]: | |
| """Yield root AGENT spans from OTLP JSON.""" | |
| for resource_span in resource_spans: | |
| for scope_spans in resource_span.get("scopeSpans", []): | |
| for span in scope_spans.get("spans", []): | |
| kind = next( | |
| ( | |
| item.get("value", {}).get("stringValue") | |
| for item in span.get("attributes", []) | |
| if item.get("key") == "openinference.span.kind" | |
| ), | |
| None, | |
| ) | |
| if not span.get("parentSpanId") and kind == "AGENT": | |
| yield span | |
| def _inject_resource_attributes(resource_spans: list[dict[str, Any]], extra: Mapping[str, str | None]) -> None: | |
| for resource_span in resource_spans: | |
| attributes = resource_span.setdefault("resource", {}).setdefault("attributes", []) | |
| existing = {attribute.get("key") for attribute in attributes} | |
| for key, value in extra.items(): | |
| if value is not None and key not in existing: | |
| attributes.append({"key": key, "value": {"stringValue": value}}) |
🤖 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 `@plugins/nemo-insights/testbed/adapters.py` around lines 475 - 519, Update
_inject_harbor_root_attributes and _inject_resource_attributes to build their
existing-key sets with item.get("key")/attribute.get("key") instead of direct
indexing, so malformed attribute entries do not raise KeyError and abort export.
Preserve the current deduplication and conditional injection behavior.
Signed-off-by: Gaia Di Lorenzo <gaia.dilorenzo01@gmail.com>
Signed-off-by: Gaia Di Lorenzo <gaia.dilorenzo01@gmail.com>
Signed-off-by: Gaia Di Lorenzo <gaia.dilorenzo01@gmail.com>
Signed-off-by: Gaia Di Lorenzo <gaia.dilorenzo01@gmail.com>
Signed-off-by: Gaia Di Lorenzo <gaia.dilorenzo01@gmail.com>
Signed-off-by: Gaia Di Lorenzo <gaia.dilorenzo01@gmail.com>
Signed-off-by: Gaia Di Lorenzo <gaia.dilorenzo01@gmail.com>
|
schuellc-nvidia
left a comment
schuellc-nvidia
left a comment
There was a problem hiding this comment.
Review: PR #1009
Reviewed the documentation changes and ran the getting-started path end to end up to the billable step.
🔴 Blocking: tau3-airline-harbor cannot resolve any tasks
plugins/nemo-insights/testbed/testbeds.toml:45-52
Step 3 of the new getting-started guide fails immediately:
ValueError: No tasks matched the filter(s) ['tau3-bench__tau3-airline-0', ...].
There are 375 tasks available in this dataset.
Example task names: ['sierra-research/tau3-bench__tau3-airline-0', ...]
Cause: Harbor filters with fnmatch(task_id.get_name(), pattern). For a Hub package
dataset, PackageTaskId.get_name() returns f"{org}/{name}" — so the ids are
sierra-research/tau3-bench__tau3-airline-0. The stanza's task_names omit the org
prefix, so all six patterns match zero tasks and Job.create() raises before any trial runs.
The unprefixed names look right when you read them next to prepare-airline.sh, which uses
the same strings — but that script matches exported directory names on disk, a different
code path from the package task-id filter.
Fix:
# Package datasets match on the fully-qualified "<org>/<task>" id.
task_names = [
"sierra-research/tau3-bench__tau3-airline-0",
"sierra-research/tau3-bench__tau3-airline-1",
"sierra-research/tau3-bench__tau3-airline-4",
"sierra-research/tau3-bench__tau3-airline-5",
"sierra-research/tau3-bench__tau3-airline-9",
"sierra-research/tau3-bench__tau3-airline-10",
]plugins/nemo-insights/tests/testbed/test_registry.py:74-81 pins the unprefixed values and
needs the same update. Worth noting the test asserted the config round-trips faithfully but
never checked the values were ones Harbor would accept, so it passed against a stanza that
could not run.
Verified by resolving the dataset against Harbor Hub (metadata fetch + filter only, no
trials): all 6 tasks resolve. pytest tests/testbed → 463 passed, 1 skipped.
🟡 Unverified: same prefix question in prepare-airline.sh
plugins/nemo-experimentalist/examples/tau3-nooa-agent/prepare-airline.sh:17-50
Step 5's script uses the same unprefixed task names. I confirmed the wrapper directory it
expects (source/tau3-bench) matches Harbor's export layout, but could not determine the
per-task directory naming without a full 375-task download, so I left it alone. If step 5
fails with a missing-task error, this is the first place to look.
🟢 Nits
docs/get-started/example-agent.mdx:38— stray article: "setting the
feature_flags.experimenttotrue" → "settingfeature_flags.experimenttotrue"..../experimentalist/components/model_config.py:23— comment readsNoOA; every other
use in the repo isNOOA.
💬 Non-blocking observations
- Feature-flag ordering (
example-agent.mdx:38): the instruction to enable
feature_flags.experimentcomes after thenemo services startcommand, so a reader
following in order has to restart the platform. Worth moving above step 2's code block. - Section 5 naming: titled "Prepare the Tau3 Airline quality dataset", but every path
istmp/tau3-airline-**smoke**/and the config isexperimentalist-smoke.yaml— leftover
from theprepare-airline-smoke.sh→prepare-airline.shrename. Not wrong, but confusing.
✅ Checked and correct
Spelling is clean across both edited docs (dictionary pass over example-agent.mdx and
testbed/README.md plus all added comments/docstrings; every flagged token was a domain term).
The prose also matches the code it describes: the "six Airline tasks" count matches the stanza,
the 20/10 train/validation split matches prepare-airline.sh, and the feature_flags.experiment
key, --task-template path, workspace, and agent name all resolve.
| In order to visualize experiment results, you need to enable the `experiment` feature flag. You can do this by editing the `packages/nmp_platform/config/local.yaml` file and setting the `feature_flags.experiment` to `true`. | ||
|
|
||
| ```yaml | ||
| feature_flags: |
There was a problem hiding this comment.
For me this flag makes no difference
There was a problem hiding this comment.
The the feature flag should be added if needed before starting the services.
|
|
||
| Now that we have a running NeMo Platform, it's time to load up some data! This example uses the τ-Bench from Sierra. It's a great representation of a simplified agent that performs a business-critical task. Our first step will be to download the tau2 repository and install the dependencies to run it. | ||
| Now that NeMo Platform is running, evaluate the | ||
| [checked-in Tau3 NOOA example agent](https://github.com/NVIDIA-NeMo/nemo-platform/tree/main/plugins/nemo-experimentalist/examples/tau3-nooa-agent) |
There was a problem hiding this comment.
Running this agent would also need to happen in a sandbox if we wanna be super save... but I guess we can ignore this for now.
2 participants
Summary by CodeRabbit
New Features
tau3-airline-harbortestbed with configurable datasets, models, tasks, and timeouts.Bug Fixes
Documentation