Skip to content

Commit 1c8f5e9

Browse files
TanishGudiseclaude
andcommitted
fix: don't load a partially written dataset file from the cache
create_dataset_files wrote <parse_func_hash>.arrow in place. When the build stopped partway (Ctrl-C, or an exception such as an unreadable response line), ArrowWriter closed the file cleanly, so it held the rows flushed so far (in batches of 1,000) and still loaded. The next run then returned that partial dataset as the cached result. Write to a temporary file (named with the process id) and move it into place only when the build finishes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 31abdb7 commit 1c8f5e9

2 files changed

Lines changed: 129 additions & 3 deletions

File tree

‎src/bespokelabs/curator/request_processor/base_request_processor.py‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
"""Base module for request processing functionality."""
22

33
import asyncio
4+
import contextlib
45
import functools
56
import glob
67
import json
@@ -30,6 +31,23 @@
3031
from datasets import Dataset
3132

3233

34+
@contextlib.contextmanager
35+
def _write_atomically(path: str):
36+
"""Yield a temporary path and move it to `path` only if the block finishes without raising.
37+
38+
An interrupted or failed write then never leaves a partial file at `path`. The temporary name
39+
includes the process id so two runs writing the same file don't remove or move each other's.
40+
"""
41+
tmp_path = f"{path}.{os.getpid()}.tmp"
42+
try:
43+
yield tmp_path
44+
except BaseException:
45+
if os.path.exists(tmp_path):
46+
os.remove(tmp_path)
47+
raise
48+
os.replace(tmp_path, path)
49+
50+
3351
class BaseRequestProcessor(ABC):
3452
"""Base class for all request processors.
3553
@@ -448,10 +466,12 @@ def create_dataset_files(
448466
total_responses_count = 0
449467
failed_responses_count = 0
450468
error_sample = []
451-
dataset_file = os.path.join(self.working_dir, f"{parse_func_hash}.arrow")
469+
final_dataset_file = os.path.join(self.working_dir, f"{parse_func_hash}.arrow")
452470
from datasets.arrow_writer import ArrowWriter
453471

454-
with ArrowWriter(path=dataset_file) as writer:
472+
# A partial dataset file would be loaded as the cached result by the next run, so write
473+
# to a temporary file and move it into place once the dataset is complete.
474+
with _write_atomically(final_dataset_file) as dataset_file, ArrowWriter(path=dataset_file) as writer:
455475
for responses_file in responses_files:
456476
with open(responses_file, "r") as f_in:
457477
for generic_response_string in f_in:
@@ -551,7 +571,7 @@ def create_dataset_files(
551571
os.remove(dataset_file)
552572
raise ValueError("Some requests do not have responses and require_all_responses is True.")
553573

554-
return self._load_from_dataset_file(dataset_file)
574+
return self._load_from_dataset_file(final_dataset_file)
555575

556576
def _load_from_dataset_file(self, dataset_file: str) -> "Dataset":
557577
from datasets import Dataset
Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
import datetime
2+
import json
3+
import os
4+
5+
import pytest
6+
from datasets import config as datasets_config
7+
8+
from bespokelabs.curator.llm.prompt_formatter import PromptFormatter
9+
from bespokelabs.curator.request_processor.config import OnlineRequestProcessorConfig
10+
from bespokelabs.curator.request_processor.online.openai_online_request_processor import OpenAIOnlineRequestProcessor
11+
from bespokelabs.curator.types.generic_request import GenericRequest
12+
from bespokelabs.curator.types.generic_response import GenericResponse
13+
14+
# The dataset writer flushes rows to disk in batches of this size. Failing after the first batch
15+
# means part of the dataset is already in the file when the build stops.
16+
_BATCH = datasets_config.DEFAULT_MAX_BATCH_SIZE
17+
_N_ROWS = _BATCH + 50
18+
_FAIL_AT_ROW = _BATCH + 10
19+
_PARSE_FUNC_HASH = "parse_func_hash"
20+
21+
22+
def _write_cached_run(working_dir, corrupt_row=None):
23+
"""Write the request and response files a finished online run leaves in its cache directory."""
24+
now = datetime.datetime.now()
25+
with open(os.path.join(working_dir, "requests_0.jsonl"), "w") as requests_file, open(os.path.join(working_dir, "responses_0.jsonl"), "w") as responses_file:
26+
for idx in range(_N_ROWS):
27+
request = GenericRequest(
28+
model="gpt-4o-mini",
29+
messages=[{"role": "user", "content": f"question {idx}"}],
30+
original_row={"question": f"question {idx}"},
31+
original_row_idx=idx,
32+
)
33+
requests_file.write(json.dumps(request.model_dump(), default=str) + "\n")
34+
if idx == corrupt_row:
35+
responses_file.write('{"response_message": "unterminated\n')
36+
continue
37+
response = GenericResponse(
38+
response_message=f"answer {idx}",
39+
raw_response={},
40+
generic_request=request,
41+
created_at=now,
42+
finished_at=now,
43+
)
44+
responses_file.write(json.dumps(response.model_dump(), default=str) + "\n")
45+
with open(os.path.join(working_dir, "metadata_0.json"), "w") as f:
46+
json.dump({"num_jobs": _N_ROWS}, f)
47+
48+
49+
def _processor(working_dir, parse_func):
50+
# create_dataset_files only needs the config, working directory and prompt formatter, so skip
51+
# __init__, which makes a network call to read rate limits.
52+
processor = object.__new__(OpenAIOnlineRequestProcessor)
53+
processor.config = OnlineRequestProcessorConfig(model="gpt-4o-mini")
54+
processor.working_dir = str(working_dir)
55+
processor.prompt_formatter = PromptFormatter(model_name="gpt-4o-mini", prompt_func=lambda row: row["question"], parse_func=parse_func)
56+
return processor
57+
58+
59+
def _parse(row, response):
60+
return {"question": row["question"], "answer": response}
61+
62+
63+
def _interrupting_parse():
64+
calls = {"n": 0}
65+
66+
def parse(row, response):
67+
calls["n"] += 1
68+
if calls["n"] == _FAIL_AT_ROW:
69+
raise KeyboardInterrupt
70+
return _parse(row, response)
71+
72+
return parse
73+
74+
75+
@pytest.mark.parametrize(
76+
"corrupt_row, parse_func, error",
77+
[
78+
pytest.param(None, _interrupting_parse, KeyboardInterrupt, id="keyboard_interrupt"),
79+
pytest.param(_FAIL_AT_ROW, lambda: _parse, ValueError, id="unreadable_response_line"),
80+
],
81+
)
82+
def test_interrupted_dataset_build_is_not_loaded_as_cached_dataset(tmp_path, corrupt_row, parse_func, error):
83+
"""A dataset build that stops partway must not leave a dataset file that the next run loads from the cache."""
84+
_write_cached_run(tmp_path, corrupt_row=corrupt_row)
85+
86+
with pytest.raises(error):
87+
_processor(tmp_path, parse_func()).create_dataset_files(_PARSE_FUNC_HASH)
88+
89+
assert _processor(tmp_path, _parse).attempt_loading_cached_dataset(_PARSE_FUNC_HASH) is None
90+
# No partial dataset or temporary file is left in the cache directory.
91+
assert sorted(os.listdir(tmp_path)) == ["metadata_0.json", "requests_0.jsonl", "responses_0.jsonl"]
92+
93+
94+
def test_dataset_is_rebuilt_from_cached_responses_after_interrupted_build(tmp_path):
95+
_write_cached_run(tmp_path)
96+
with pytest.raises(KeyboardInterrupt):
97+
_processor(tmp_path, _interrupting_parse()).create_dataset_files(_PARSE_FUNC_HASH)
98+
99+
dataset = _processor(tmp_path, _parse).create_dataset_files(_PARSE_FUNC_HASH)
100+
101+
assert len(dataset) == _N_ROWS
102+
cached = _processor(tmp_path, _parse).attempt_loading_cached_dataset(_PARSE_FUNC_HASH)
103+
assert cached["answer"] == [f"answer {idx}" for idx in range(_N_ROWS)]
104+
assert sorted(os.listdir(tmp_path)) == sorted(
105+
[f"{_PARSE_FUNC_HASH}.arrow", "failed_requests.jsonl", "metadata_0.json", "requests_0.jsonl", "responses_0.jsonl"]
106+
)

0 commit comments

Comments
 (0)