Fix avg_llm_latency ATIF evaluator always reporting 0.0

[AnishPatel526]

Fix avg_llm_latency ATIF evaluator always reporting 0.0

Reference Issues/PRs

Closes #2116.

What does this implement/fix? Explain your changes.

AverageLLMLatencyAtifEvaluator computed per-step LLM latency from
step.extra["span_event_timestamp"] (an ISO start paired with step.timestamp
as the end). NAT's ATOF-to-ATIF converter never writes that key — it records
per-step timing under step.extra["invocation"] as start_timestamp /
end_timestamp in epoch seconds (AtifInvocationInfo, written by
atof_to_atif_converter.py). So on real converter output the evaluator found no
timing, latencies stayed empty, and the score was always 0.0.

Fix

Read the timing the converter actually emits. A new helper _step_llm_latency
prefers step.extra["invocation"] (end_timestamp - start_timestamp), and falls
back to the legacy span_event_timestamp + step.timestamp pair so producers
that emit that shape (and the existing tests for it) keep working. Steps with
neither source are skipped rather than recorded as a spurious zero.

Before / after

For a real ATIF agent step:

step.extra = {"invocation": {"start_timestamp": 1000.0, "end_timestamp": 1003.5}}

• Before: span_event_timestamp absent → latencies == [] → score == 0.0.
• After: invocation used → score == 3.5.

The legacy span_event_timestamp path is unchanged (still 5.0 for the existing
5-second fixture).

Testing

Added test_evaluate_atif_item_uses_invocation_timing in
packages/nvidia_nat_profiler/tests/profiler/test_runtime_evaluator_atif.py,
which uses the converter's real extra["invocation"] shape and asserts a
non-zero average. It fails on develop (score 0.0) and passes with this
change. The existing span_event_timestamp tests continue to pass via the
fallback path. ruff check and ruff format are clean on both changed files.

(The evaluator logic was verified out-of-tree as well: invocation input yields
3.5, the legacy input yields 5.0, and the pre-fix code returns 0.0 on
invocation input — confirming the reported bug and the fix.)

Checklist

• "closes avg_llm_latency evaluator always reports 0.0 — ATIF evaluator reads a key the ATIF converter never writes #2116" is in the PR body.
• New/changed code is tested (regression test that fails before the fix).
• Documentation updated where applicable (N/A — behavior fix, no API change).
• Commits are signed off (DCO).

Summary by CodeRabbit

• Bug Fixes

  • Improved LLM latency measurement by preferring per-step invocation start/end timestamps when available.
  • Retains the prior timestamp-based calculation as a fallback.
  • Automatically ignores steps with invalid or non-finite/incomplete timing data to prevent incorrect latencies.

• Tests

  • Added regression coverage to verify invocation timing takes precedence over legacy timestamp fields.
  • Added edge-case tests for non-finite or unconvertible invocation timings, confirming results are skipped (zero calls, no recorded latencies).

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 802c42e0-a5c4-4f09-ac6e-caf9547dd2b5

📥 Commits

Reviewing files that changed from the base of the PR and between 7939f4b and 2bfcc68.

📒 Files selected for processing (1)

• packages/nvidia_nat_profiler/tests/profiler/test_runtime_evaluator_atif.py

---

Walkthrough

The ATIF average LLM latency evaluator now prioritizes invocation epoch timestamps, falls back to legacy timestamps, skips invalid timing data, and includes regression tests for invocation timing and non-finite values.

Changes

ATIF latency evaluation

Layer / File(s)	Summary
Latency extraction helper packages/nvidia_nat_profiler/src/nat/plugins/profiler/runtime_evaluator/atif_evaluate.py	Adds a typed helper that computes non-negative latency from invocation timestamps and falls back to legacy ISO timestamp fields.
Evaluator integration and regression coverage packages/nvidia_nat_profiler/src/nat/plugins/profiler/runtime_evaluator/atif_evaluate.py, packages/nvidia_nat_profiler/tests/profiler/test_runtime_evaluator_atif.py	Updates the evaluator to record only valid helper results and tests invocation-based latency, call counts, recorded values, and non-finite timing exclusion.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise, imperative, and accurately describes the main fix.
Linked Issues check	✅ Passed	The changes address #2116 by using converter-emitted invocation timing and adding regression coverage.
Out of Scope Changes check	✅ Passed	The code and tests stay focused on avg_llm_latency timing handling with no clear unrelated changes.
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 4

🤖 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
`@packages/nvidia_nat_profiler/src/nat/plugins/profiler/runtime_evaluator/atif_evaluate.py`:
- Around line 37-48: Update the docstring of _step_llm_latency to remove the
rejected “NAT” term, replacing it with “the ATOF-to-ATIF converter” or the
approved full product name while preserving the existing meaning.
- Around line 80-82: Update the evaluator class docstring near the latency
aggregation logic to document the new timing precedence: use extra["invocation"]
when available, fall back to span_event_timestamp, and skip steps only when
neither timing source exists.
- Around line 52-55: Update the timestamp validation in the invocation duration
evaluation to reject non-finite values and integers that cannot be safely
converted to float before calling float(start) or float(end). Skip such
invocations rather than computing a score, while preserving the existing
non-negative duration behavior for valid finite timestamps.

In `@packages/nvidia_nat_profiler/tests/profiler/test_runtime_evaluator_atif.py`:
- Around line 122-126: Update the docstring of
test_evaluate_atif_item_uses_invocation_timing so its opening sentence is
concise and ends with a period, and format the avg_llm_latency identifier with
backticks. Preserve the test’s existing meaning and regression context.

🪄 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: fa37b017-cb84-4eb4-bf28-ed48528318d4

📥 Commits

Reviewing files that changed from the base of the PR and between baf355b and ccfdecb.

📒 Files selected for processing (2)

• packages/nvidia_nat_profiler/src/nat/plugins/profiler/runtime_evaluator/atif_evaluate.py
• packages/nvidia_nat_profiler/tests/profiler/test_runtime_evaluator_atif.py

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

packages/nvidia_nat_profiler/tests/profiler/test_runtime_evaluator_atif.py (1)

122-146: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Test invocation-timing precedence explicitly.

This verifies that invocation timestamps work, but not that they take precedence over legacy timing. Add a conflicting extra["span_event_timestamp"] value and keep the expected latency at 3.5; otherwise a fallback-first implementation could still pass this regression test.

🤖 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 `@packages/nvidia_nat_profiler/tests/profiler/test_runtime_evaluator_atif.py`
around lines 122 - 146, Update test_evaluate_atif_item_uses_invocation_timing to
include a conflicting extra["span_event_timestamp"] value alongside
extra["invocation"], while keeping the expected latency and reasoning assertions
at 3.5 and one call. This must verify that invocation timing takes precedence
over legacy span-event timing.

🤖 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 `@packages/nvidia_nat_profiler/tests/profiler/test_runtime_evaluator_atif.py`:
- Around line 149-154: Update the docstring of
test_evaluate_atif_item_skips_non_finite_invocation_timing to match the cases
actually exercised: either add a large-integer overflow scenario to the test or
remove the “and overflowing” claim, while preserving the existing NaN and inf
coverage.

---

Nitpick comments:
In `@packages/nvidia_nat_profiler/tests/profiler/test_runtime_evaluator_atif.py`:
- Around line 122-146: Update test_evaluate_atif_item_uses_invocation_timing to
include a conflicting extra["span_event_timestamp"] value alongside
extra["invocation"], while keeping the expected latency and reasoning assertions
at 3.5 and one call. This must verify that invocation timing takes precedence
over legacy span-event timing.

🪄 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: 9a1fe0de-be7e-4470-b5fa-7feb22d0e167

📥 Commits

Reviewing files that changed from the base of the PR and between ccfdecb and 7939f4b.

📒 Files selected for processing (2)

• packages/nvidia_nat_profiler/src/nat/plugins/profiler/runtime_evaluator/atif_evaluate.py
• packages/nvidia_nat_profiler/tests/profiler/test_runtime_evaluator_atif.py

🚧 Files skipped from review as they are similar to previous changes (1)

• packages/nvidia_nat_profiler/src/nat/plugins/profiler/runtime_evaluator/atif_evaluate.py

[AnishPatel526]

This is a non-breaking bug fix (avg_llm_latency always returned 0.0 because the evaluator read span_event_timestamp, which the ATOF-to-ATIF converter never writes). Could a maintainer apply the bug and non-breaking labels so the Label Checker can pass? Happy to adjust if you'd categorize it differently.

[AnishPatel526]

Hi! Just wanted to follow up on this contribution!