Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ litellm[caching]==1.83.14
math-verify[antlr4_9_3]
mcp
numpy
openai
openai[aiohttp]
openpyxl>=3.1.0
pandas>=2.0.0
pyxlsb>=1.0.10
Expand Down
178 changes: 156 additions & 22 deletions nemo_skills/inference/generate.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,22 @@
import asyncio
import json
import logging
import os
import random
import shutil
import subprocess
import sys
import time

# Prefer uvloop when available to reduce event-loop scheduling overhead.
# Set NEMO_SKILLS_DISABLE_UVLOOP to use the default asyncio event loop.
if not os.environ.get("NEMO_SKILLS_DISABLE_UVLOOP"):
try:
import uvloop

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need to be added to requirements?


uvloop.install()
except ImportError:
pass
from copy import deepcopy
from dataclasses import asdict, field, is_dataclass
from pathlib import Path
Expand Down Expand Up @@ -125,6 +136,12 @@ class GenerationTaskConfig:
# maximum number of concurrent requests to the server for the async loop
# if sync loop is used, this is the batch size
max_concurrent_requests: int = 512
# Target task-creation rate while initially filling the async worker pool.
# Tasks are created in short batches to limit connection bursts. Values <= 0 disable ramping.
ramp_rate_per_sec: int = 4000
# Number of background coroutines that postprocess and optionally evaluate completed datapoints.
# Increase this when per-datapoint evaluation benefits from additional concurrency.
postprocess_workers: int = 16
# chunk the dataset into equal sized parts and index into them
num_chunks: int | None = None # if specified, will split the data into chunks and only generate for one chunk
chunk_id: int | None = None # if specified, will index the specified chunk only
Expand Down Expand Up @@ -409,6 +426,9 @@ def __init__(self, cfg: GenerationTaskConfig):

# output_lock will be initialized when async_loop is called
self.output_lock = None
# Pipeline queues are set by async_loop; None selects inline processing for other callers.
self._raw_queue = None
self._output_queue = None

def setup_prompt(self):
if self.cfg.prompt_config is None:
Expand Down Expand Up @@ -849,16 +869,17 @@ async def _generate_and_save_datapoint(self, data_point, all_data, fout, pbar):
output["generation_end_time"] = end_time
output["generation_time"] = end_time - start_time

await self.postprocess_single_output(output, data_point)

# evaluate single-data point if requested and evaluator supports that
if self.should_run_evaluation and self.evaluator:
output = await self.evaluate_single_datapoint({**data_point, **output})

# Thread-safe output writing
async with self.output_lock:
self.dump_outputs([output], [data_point], fout)
pbar.update(1)
# Async-loop callers enqueue completed generations for postprocessing and batched output.
if self._raw_queue is not None:
await self._raw_queue.put((output, data_point))
else:
# Other callers postprocess and write inline.
await self.postprocess_single_output(output, data_point)
if self.should_run_evaluation and self.evaluator:
output = await self.evaluate_single_datapoint({**data_point, **output})
async with self.output_lock:
self.dump_outputs([output], [data_point], fout)
pbar.update(1)

async def async_loop(self, data):
"""Async loop to generate generations using asyncio."""
Expand All @@ -881,7 +902,8 @@ async def async_loop(self, data):

pbar = tqdm(total=len(remaining_data_points), desc="Remaining generations")

with open(self.cfg.output_file + "-async", "at", encoding="utf-8", buffering=1) as fout:
# The writer flushes after each batch, so default buffering avoids per-record flushes.
with open(self.cfg.output_file + "-async", "at", encoding="utf-8") as fout:
# Dump prefilled data first
if len(prefilled_data_points) > 0:
for output, data_point in zip(prefilled_outputs, prefilled_data_points):
Expand All @@ -893,15 +915,119 @@ async def async_loop(self, data):
async with self.output_lock:
self.dump_outputs(prefilled_outputs, prefilled_data_points, fout)

# Create tasks for all remaining data points
tasks = []
for data_point in remaining_data_points:
task = asyncio.create_task(self._generate_and_save_datapoint(data_point, data, fout, pbar))
tasks.append(task)

# Wait for all tasks to complete
if tasks:
await asyncio.gather(*tasks)
# Generation tasks -> raw queue -> postprocessors -> output queue -> batched writer.
# Bounded queues provide backpressure; None sentinels stop each processing stage.
queue_cap = max(self.cfg.max_concurrent_requests * 2, 1024)
self._raw_queue: asyncio.Queue = asyncio.Queue(maxsize=queue_cap)
self._output_queue = asyncio.Queue(maxsize=queue_cap)
num_processors = max(1, int(self.cfg.postprocess_workers))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Reject invalid postprocess_workers instead of silently clamping.

Line 1001 turns 0/negative user input into 1, so a bad config silently runs with different behavior than requested. Prefer an explicit ValueError.

Proposed fix
-            num_processors = max(1, int(self.cfg.postprocess_workers))
+            num_processors = int(self.cfg.postprocess_workers)
+            if num_processors < 1:
+                raise ValueError(f"postprocess_workers must be >= 1, got {self.cfg.postprocess_workers}")

As per coding guidelines, “Avoid cases when user-passed parameters are unused” and “the code should fail” for unsupported arguments.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
num_processors = max(1, int(self.cfg.postprocess_workers))
num_processors = int(self.cfg.postprocess_workers)
if num_processors < 1:
raise ValueError(f"postprocess_workers must be >= 1, got {self.cfg.postprocess_workers}")
🤖 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 `@nemo_skills/inference/generate.py` at line 1001, The postprocess worker count
is being silently clamped in generate.py, which hides invalid user input. Update
the logic around self.cfg.postprocess_workers in the generation path to validate
the configured value explicitly and raise a ValueError when it is 0 or negative
instead of forcing num_processors to 1. Keep the check close to the
num_processors assignment so the failure is immediate and easy to trace.

Source: Coding guidelines


async def _processor():
# Each processor receives its own sentinel after all raw outputs are queued.
while True:
item = await self._raw_queue.get()
if item is None:
return
output, data_point = item
try:
await self.postprocess_single_output(output, data_point)
if self.should_run_evaluation and self.evaluator:
output = await self.evaluate_single_datapoint({**data_point, **output})
except Exception as e:
pos = data_point.get(self.cfg.async_position_key, "?")
LOG.exception("postprocess/evaluate failed for async_pos=%s", pos)
# Preserve ordering metadata and failure details for the output record.
if self.cfg.async_position_key in data_point:
output[self.cfg.async_position_key] = data_point[self.cfg.async_position_key]
output.setdefault("postprocess_error", repr(e))
await self._output_queue.put((output, data_point))
Comment on lines +932 to +943

@coderabbitai coderabbitai Bot Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Don’t swallow datapoint failures; raise after draining the pipeline.

These handlers log and continue. A postprocess/eval failure can enqueue raw output without _async_position; a generation failure can skip a position entirely. restore_async_order() then fails late or writes incomplete results. Collect failures while draining queues, then raise before restoring/removing the async file.

Proposed pattern
+            errors = []
+
             async def _processor():
@@
-                    try:
+                    try:
                         await self.postprocess_single_output(output, data_point)
                         if self.should_run_evaluation and self.evaluator:
                             output = await self.evaluate_single_datapoint({**data_point, **output})
-                    except Exception:
-                        pos = data_point.get(self.cfg.async_position_key, "?")
+                    except Exception as exc:
+                        errors.append(exc)
+                        pos = data_point[self.cfg.async_position_key]
                         LOG.exception("postprocess/evaluate failed for async_pos=%s", pos)
+                        continue
                     await self._output_queue.put((output, data_point))
@@
             async def _safe_one(dp):
                 try:
                     await self._generate_and_save_datapoint(dp, data, fout, pbar)
-                except Exception:
-                    pos = dp.get(self.cfg.async_position_key, "?")
+                except Exception as exc:
+                    errors.append(exc)
+                    pos = dp[self.cfg.async_position_key]
                     LOG.exception("datapoint async_pos=%s crashed", pos)
@@
             await self._output_queue.put(None)
             await writer_task
+            if errors:
+                raise RuntimeError(f"{len(errors)} datapoint(s) failed during async generation")
             self._raw_queue = None
             self._output_queue = None

As per coding guidelines, “Don’t catch exceptions when we don’t expect them to be normally raised” and prefer direct key access when keys are expected.

Also applies to: 1072-1077

🤖 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 `@nemo_skills/inference/generate.py` around lines 1013 - 1020, Stop swallowing
datapoint processing failures in the async output path: the exception handling
around postprocess/evaluate in generate.py currently logs and continues, which
can enqueue incomplete results and break restore_async_order() later. Update the
async draining logic in the relevant handler(s) around
postprocess_single_output, evaluate_single_datapoint, and the similar block near
the other referenced section to collect any failure state while draining the
queue, keep processing until the pipeline is drained, and then raise the
captured error before restoring/removing the async file. Also use direct access
for expected keys like _async_position instead of .get where the position must
exist.

Source: Coding guidelines

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gwarmstrong not sure if this is correct but if it is, we should probably fail on exceptions

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kipok, agreed — this is a real gap. Right now _processor() and _safe_one() both catch broad Exception, log it, and keep going:

  • A postprocess/eval failure in _processor() logs and then still does await self._output_queue.put((output, data_point)) with whatever partial output exists — if postprocess_single_output failed before setting _async_position-related fields or before merging original_data_point, that record can be malformed or missing the position key entirely.
  • A crash in _generate_and_save_datapoint inside _safe_one() is caught, logged, and silently dropped — no placeholder is emitted for that position at all, so that datapoint's slot is simply missing from the async file.

Both cases mean restore_async_order() either writes an incomplete/corrupted output file or (per the current implementation) silently drops that position with just a warning log — so a large async run could finish "successfully" while quietly missing results, which is easy to miss without inspecting logs closely.

This does match the coding guideline of not swallowing unexpected exceptions — the fix should be to track these failures while draining the pipeline (or emit clearly-marked placeholder records with the position preserved) and then raise once the queues are fully drained, rather than letting the run report success with silently missing/malformed data. Happy to help draft that once you and @gwarmstrong agree on the exact approach (hard fail vs. always-emit-placeholder-then-fail).


def _flush_batch(batch):
# Use the overridable hook so subclasses retain custom serialization.
self.dump_outputs([o for o, _dp in batch], [dp for _o, dp in batch], fout)
pbar.update(len(batch))
fout.flush()

async def _writer():
# Block for the first item, then drain and flush all currently queued items as one batch.
while True:
item = await self._output_queue.get()
if item is None:
return
batch = [item]
stop = False
# Drain anything that's already enqueued.
while not self._output_queue.empty():
nxt = self._output_queue.get_nowait()
if nxt is None:
stop = True
break
batch.append(nxt)
_flush_batch(batch)
if stop:
return

processor_tasks = [asyncio.create_task(_processor()) for _ in range(num_processors)]
writer_task = asyncio.create_task(_writer())

# Bound both request concurrency and allocated task state to max_in_flight.
# Each task handles its own error so other datapoints continue processing.
max_in_flight = self.cfg.max_concurrent_requests

async def _safe_one(dp):
try:
await self._generate_and_save_datapoint(dp, data, fout, pbar)
except Exception as e:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess similar here, why do we want to catch all exceptions?

pos = dp.get(self.cfg.async_position_key, "?")
LOG.exception("datapoint async_pos=%s crashed", pos)
# Emit one placeholder per failed datapoint to preserve positional alignment.
# Route it through the raw queue so it follows normal postprocessing.
if self._raw_queue is not None:
await self._raw_queue.put(({"generation": "", "generation_error": repr(e)}, dp))

data_iter = iter(remaining_data_points)
in_flight: set = set()

# Fill the concurrency pool at the configured rate to limit initial connection bursts.
ramp_rate = self.cfg.ramp_rate_per_sec
if ramp_rate > 0:
# Quarter-second batches keep the ramp smooth while filling the pool promptly.
ramp_batch = max(50, ramp_rate // 4)
ramp_interval = ramp_batch / ramp_rate
Comment on lines +992 to +996

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Honor low ramp_rate_per_sec values.

With ramp_rate_per_sec=1, Line 995 still creates an initial burst of 50 tasks before sleeping, which violates the configured ramp limit and can overload endpoints the knob is meant to protect.

Proposed fix
-                ramp_batch = max(50, ramp_rate // 4)
+                ramp_batch = max(1, ramp_rate // 4)
                 ramp_interval = ramp_batch / ramp_rate

As per coding guidelines, user-passed parameters should not be silently ignored or changed.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
ramp_rate = self.cfg.ramp_rate_per_sec
if ramp_rate > 0:
# Quarter-second batches keep the ramp smooth while filling the pool promptly.
ramp_batch = max(50, ramp_rate // 4)
ramp_interval = ramp_batch / ramp_rate
ramp_rate = self.cfg.ramp_rate_per_sec
if ramp_rate > 0:
# Quarter-second batches keep the ramp smooth while filling the pool promptly.
ramp_batch = max(1, ramp_rate // 4)
ramp_interval = ramp_batch / ramp_rate
🤖 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 `@nemo_skills/inference/generate.py` around lines 992 - 996, The ramp
scheduling in generate.py ignores very small ramp_rate_per_sec values because
ramp_batch is forced to at least 50 in the ramp logic. Update the ramp
calculation around ramp_rate, ramp_batch, and ramp_interval so user-provided low
rates are honored exactly, without a minimum burst that exceeds the configured
limit. Keep the behavior in the same ramp control block and adjust the batching
formula to derive batch size directly from ramp_rate_per_sec while preserving
the smooth quarter-second pacing intent.

Source: Coding guidelines

else:
ramp_batch = max_in_flight # instant prime
ramp_interval = 0.0

primed = 0
for dp in data_iter:
in_flight.add(asyncio.create_task(_safe_one(dp)))
primed += 1
if primed >= max_in_flight:
break
if ramp_interval > 0 and primed % ramp_batch == 0:
await asyncio.sleep(ramp_interval)

# Refill one worker slot for each completed task.
while in_flight:
done, in_flight = await asyncio.wait(
in_flight,
return_when=asyncio.FIRST_COMPLETED,
)
for _ in done:
try:
dp = next(data_iter)
except StopIteration:
continue
in_flight.add(asyncio.create_task(_safe_one(dp)))

# Stop processors after all raw outputs are queued, then stop the writer after processors finish.
for _ in range(num_processors):
await self._raw_queue.put(None)
await asyncio.gather(*processor_tasks)
await self._output_queue.put(None)
await writer_task
self._raw_queue = None
self._output_queue = None

pbar.close()

Expand All @@ -912,13 +1038,21 @@ def restore_async_order(self):
with open(self.cfg.output_file + "-async", "rt", encoding="utf-8") as fin:
generations = [json.loads(line) for line in fin]

ordered_generations = [None] * len(generations)
# Original positions can be sparse, so size from the maximum and skip missing entries.
key = self.cfg.async_position_key
positions = [gen[key] for gen in generations if key in gen]
ordered_generations = [None] * ((max(positions) + 1) if positions else 0)
for gen_dict in generations:
async_pos = gen_dict.pop(self.cfg.async_position_key)
async_pos = gen_dict.pop(key, None)
if async_pos is None:
LOG.warning("async record missing %s, dropping from reorder", key)
continue
ordered_generations[async_pos] = gen_dict
Comment on lines +1042 to 1050

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Don't drop positionless async records during finalization.

If a record reaches reorder without self.cfg.async_position_key, this branch logs a warning, omits the row from the final JSONL, and then deletes the only recovery file. That is silent data loss.

Proposed fix
         for gen_dict in generations:
-            async_pos = gen_dict.pop(key, None)
-            if async_pos is None:
-                LOG.warning("async record missing %s, dropping from reorder", key)
-                continue
+            if key not in gen_dict:
+                raise ValueError(f"async record missing required key {key!r} during reorder")
+            async_pos = gen_dict.pop(key)
             ordered_generations[async_pos] = gen_dict

As per coding guidelines, “Don't use .get() for accessing dictionary keys if the code expects them to be present” and “Errors should never pass silently.”

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
key = self.cfg.async_position_key
positions = [gen[key] for gen in generations if key in gen]
ordered_generations = [None] * ((max(positions) + 1) if positions else 0)
for gen_dict in generations:
async_pos = gen_dict.pop(self.cfg.async_position_key)
async_pos = gen_dict.pop(key, None)
if async_pos is None:
LOG.warning("async record missing %s, dropping from reorder", key)
continue
ordered_generations[async_pos] = gen_dict
key = self.cfg.async_position_key
positions = [gen[key] for gen in generations if key in gen]
ordered_generations = [None] * ((max(positions) + 1) if positions else 0)
for gen_dict in generations:
if key not in gen_dict:
raise ValueError(f"async record missing required key {key!r} during reorder")
async_pos = gen_dict.pop(key)
ordered_generations[async_pos] = gen_dict
🤖 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 `@nemo_skills/inference/generate.py` around lines 1176 - 1184, The reorder
logic in generate.py is dropping async records that are missing
self.cfg.async_position_key, which causes silent data loss during finalization.
Update the reordering path around ordered_generations and the gen_dict.pop(key,
None) handling so positionless records are preserved instead of skipped, and
make the failure explicit if the key is truly required. Keep the recovery flow
intact in the finalization path and ensure generate/reorder does not delete the
only recoverable data when a record lacks the async position key.

Source: Coding guidelines


with open(self.cfg.output_file, "wt", encoding="utf-8") as fout:
for gen_dict in ordered_generations:
if gen_dict is None:
continue
fout.write(json.dumps(gen_dict) + "\n")

Path(self.cfg.output_file + "-async").unlink()
Expand Down
3 changes: 3 additions & 0 deletions nemo_skills/inference/model/azure.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@

class AzureOpenAIModel(OpenAIModel):
MODEL_PROVIDER = "azure"
# Azure requires provider-specific endpoint and authentication handling
# from litellm.
SUPPORTS_NATIVE_OPENAI = False

def __init__(
self,
Expand Down
Loading
Loading