Skip to content

Commit d74d4ef

Browse files
committed
fix(dataset): align validation with the platform data-format rules doc
Reviewed against the official text-tuning data rules; fixes two confirmed mismatches and fills enforcement gaps: - thinking: exempt assistant messages carrying tool_calls from the THINK_TAG_NOT_LAST check — the spec's tool+thinking combo example puts <think> on a non-last assistant and was previously false-flagged - DPO support matrix: reject image/video content items, tools, tool_calls and role:tool (DPO_UNSUPPORTED_ELEMENT); also scan chosen/rejected - DPO: messages not ending with user upgraded warning -> error - OpenAI migration: name/weight upgraded warning -> error (spec: must not carry); drop dead record-level name branch - tool_call_id: unmatched tool response upgraded warning -> error (one-to-one per spec); new TOOL_CALL_NO_RESPONSE warning for orphan calls - loss_weight: validate range at message level too; warn when placed on anything but the last assistant message (LOSS_WEIGHT_PLACEMENT) - video params: fps/sample_fps must be within [0.1, 10] (INVALID_VIDEO_FPS); mode-mismatched params warned (VIDEO_PARAM_MODE_MISMATCH); video_start/video_end type-checked - zip: skip macOS packaging metadata (__MACOSX/, .DS_Store, ._*) in filename constraints and image counting to stop false failures on Finder-created archives Tests 45 -> 59 covering every new/changed rule, including a replica of the spec's official tool+thinking example.
1 parent e7422bd commit d74d4ef

4 files changed

Lines changed: 394 additions & 62 deletions

File tree

packages/core/src/dataset/validate/schemas/chatml.ts

Lines changed: 138 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,69 @@ import type { RecordSchemaSpec } from "./types.ts";
2020

2121
const VALID_ROLES = new Set(["system", "user", "assistant", "tool"]);
2222

23+
/** Platform bounds for video sampling rate params (`fps` / `sample_fps`). */
24+
const VIDEO_FPS_MIN = 0.1;
25+
const VIDEO_FPS_MAX = 10;
26+
27+
/**
28+
* Validate the sampling/clipping params carried by a video content item.
29+
* Mode rules (platform spec):
30+
* - path mode (video: string): `fps`, `video_start`, `video_end` allowed; `sample_fps` is not
31+
* - frame-list mode (video: string[]): `sample_fps` allowed; `fps` / `video_start` / `video_end` are not
32+
* `fps` / `sample_fps` must be numbers within [0.1, 10] when present.
33+
*/
34+
function inspectVideoParams(
35+
item: Record<string, unknown>,
36+
isFrameList: boolean,
37+
lineNo: number,
38+
itemPath: string,
39+
): ValidationIssue[] {
40+
const out: ValidationIssue[] = [];
41+
const checkFpsRange = (field: "fps" | "sample_fps"): void => {
42+
if (!(field in item)) return;
43+
const value = item[field];
44+
if (typeof value !== "number" || value < VIDEO_FPS_MIN || value > VIDEO_FPS_MAX) {
45+
out.push(
46+
makeIssue(
47+
"error",
48+
"INVALID_VIDEO_FPS",
49+
`"${field}" must be a number between ${VIDEO_FPS_MIN} and ${VIDEO_FPS_MAX} (got ${JSON.stringify(value)}).`,
50+
{ line: lineNo, path: `${itemPath}.${field}` },
51+
),
52+
);
53+
}
54+
};
55+
checkFpsRange("fps");
56+
checkFpsRange("sample_fps");
57+
58+
const wrongModeFields = isFrameList ? ["fps", "video_start", "video_end"] : ["sample_fps"];
59+
const modeName = isFrameList ? "frame-list" : "file-path";
60+
for (const field of wrongModeFields) {
61+
if (field in item) {
62+
out.push(
63+
makeIssue(
64+
"warning",
65+
"VIDEO_PARAM_MODE_MISMATCH",
66+
`"${field}" does not apply to ${modeName} video mode and will be ignored by the platform.`,
67+
{ line: lineNo, path: `${itemPath}.${field}` },
68+
),
69+
);
70+
}
71+
}
72+
73+
for (const field of ["video_start", "video_end"] as const) {
74+
if (field in item && !isFrameList && typeof item[field] !== "number") {
75+
out.push(
76+
makeIssue("error", "INVALID_VIDEO_CLIP_TIME", `"${field}" must be a number (seconds).`, {
77+
line: lineNo,
78+
path: `${itemPath}.${field}`,
79+
}),
80+
);
81+
}
82+
}
83+
return out;
84+
}
85+
2386
/**
2487
* Validate a content field that may be:
2588
* - A plain string (legacy format)
@@ -112,19 +175,22 @@ export function inspectContentField(
112175
{ line: lineNo, path: `${itemPath}.video` },
113176
),
114177
);
115-
} else if (Array.isArray(video)) {
116-
for (let frameIdx = 0; frameIdx < video.length; frameIdx++) {
117-
if (typeof video[frameIdx] !== "string") {
118-
out.push(
119-
makeIssue(
120-
"error",
121-
"INVALID_VIDEO_FRAME",
122-
`Video frame list item at index ${frameIdx} must be a string.`,
123-
{ line: lineNo, path: `${itemPath}.video[${frameIdx}]` },
124-
),
125-
);
178+
} else {
179+
if (Array.isArray(video)) {
180+
for (let frameIdx = 0; frameIdx < video.length; frameIdx++) {
181+
if (typeof video[frameIdx] !== "string") {
182+
out.push(
183+
makeIssue(
184+
"error",
185+
"INVALID_VIDEO_FRAME",
186+
`Video frame list item at index ${frameIdx} must be a string.`,
187+
{ line: lineNo, path: `${itemPath}.video[${frameIdx}]` },
188+
),
189+
);
190+
}
126191
}
127192
}
193+
out.push(...inspectVideoParams(obj, Array.isArray(video), lineNo, itemPath));
128194
}
129195
}
130196
}
@@ -283,23 +349,24 @@ export function inspectMessageObject(
283349
out.push(...inspectToolCalls(record.tool_calls, lineNo, `${path}.tool_calls`));
284350
}
285351

286-
// OpenAI migration guard: name / weight are not supported by Bailian
352+
// OpenAI migration guard: the platform rejects data carrying name / weight
287353
if ("name" in record) {
288354
out.push(
289355
makeIssue(
290-
"warning",
356+
"error",
291357
"UNSUPPORTED_FIELD_NAME",
292-
`Field "name" is not supported by Bailian. Remove it when migrating from OpenAI/Azure.`,
358+
`Field "name" is not supported by Bailian and must be removed when migrating from OpenAI/Azure.`,
293359
{ line: lineNo, path: `${path}.name` },
294360
),
295361
);
296362
}
297363
if ("weight" in record) {
298364
out.push(
299365
makeIssue(
300-
"warning",
366+
"error",
301367
"UNSUPPORTED_FIELD_WEIGHT",
302-
`Field "weight" is not supported by Bailian. Remove it when migrating from OpenAI/Azure.`,
368+
`Field "weight" is not supported by Bailian and must be removed when migrating from OpenAI/Azure. ` +
369+
`All assistant outputs are trained; per-line importance uses "loss_weight" (invite-only).`,
303370
{ line: lineNo, path: `${path}.weight` },
304371
),
305372
);
@@ -419,73 +486,104 @@ export function inspectChatMLRecord(
419486
);
420487
}
421488

422-
// tool_call_id correspondence: every tool response should reference a known call id
489+
// tool_call_id correspondence must be one-to-one (platform spec):
490+
// every tool response must reference a known call id (hard error), and every
491+
// tool_call should receive a response (advisory — trailing calls are dubious
492+
// in training data but we cannot rule out platform-side tolerance).
423493
for (const responseId of toolResponseIds) {
424494
if (!toolCallIds.has(responseId)) {
425495
out.push(
426496
makeIssue(
427-
"warning",
497+
"error",
428498
"TOOL_CALL_ID_UNMATCHED",
429499
`tool message references tool_call_id "${responseId}" which does not match any assistant tool_calls[].id.`,
430500
{ line: lineNo, path: "messages" },
431501
),
432502
);
433503
}
434504
}
505+
for (const callId of toolCallIds) {
506+
if (!toolResponseIds.has(callId)) {
507+
out.push(
508+
makeIssue(
509+
"warning",
510+
"TOOL_CALL_NO_RESPONSE",
511+
`assistant tool_calls[].id "${callId}" has no matching tool response message.`,
512+
{ line: lineNo, path: "messages" },
513+
),
514+
);
515+
}
516+
}
435517

436-
// thinking tag check: <think>…</think>` should only appear in the last assistant message
518+
// thinking tag check: <think>…</think> should only appear in the last
519+
// assistant message. Exemption (platform spec, tool+thinking combo): an
520+
// assistant that carries tool_calls may legitimately hold a <think> block
521+
// even when it is not the last assistant message.
437522
if (lastAssistantIdx >= 0) {
438523
for (let idx = 0; idx < messages.length; idx++) {
439524
if (idx === lastAssistantIdx) continue;
440525
const msg = messages[idx] as Record<string, unknown> | null;
441526
if (msg?.role !== "assistant") continue;
527+
if (msg && Array.isArray(msg.tool_calls)) continue;
442528
const content = msg?.content;
443529
if (contentHasThinkTag(content)) {
444530
out.push(
445531
makeIssue(
446532
"warning",
447533
"THINK_TAG_NOT_LAST",
448-
`Thinking tags (<think>…</think>) should only appear in the last assistant message, found at messages[${idx}].`,
534+
`Thinking tags (<think>…</think>) should only appear in the last assistant message ` +
535+
`(or an assistant message carrying tool_calls), found at messages[${idx}].`,
449536
{ line: lineNo, path: `messages[${idx}].content` },
450537
),
451538
);
452539
}
453540
}
454541
}
455542

456-
// loss_weight validation (record-level, invite-only parameter)
457-
if ("loss_weight" in record) {
458-
const lossWeight = record.loss_weight;
459-
if (typeof lossWeight !== "number" || lossWeight < 0 || lossWeight > 1) {
543+
// loss_weight validation (invite-only parameter).
544+
// Range is enforced wherever the field appears (record level and message
545+
// level); placement follows the spec: only the LAST assistant message line
546+
// supports loss_weight — misplaced occurrences are advisory (invite-only
547+
// semantics are account-specific, so we do not hard-fail).
548+
const checkLossWeightRange = (value: unknown, path: string): void => {
549+
if (typeof value !== "number" || value < 0 || value > 1) {
460550
out.push(
461551
makeIssue(
462552
"error",
463553
"INVALID_LOSS_WEIGHT",
464-
`"loss_weight" must be a number between 0.0 and 1.0 (got ${JSON.stringify(lossWeight)}).`,
465-
{ line: lineNo, path: "loss_weight" },
554+
`"loss_weight" must be a number between 0.0 and 1.0 (got ${JSON.stringify(value)}).`,
555+
{ line: lineNo, path },
466556
),
467557
);
468558
}
559+
};
560+
if ("loss_weight" in record) {
561+
checkLossWeightRange(record.loss_weight, "loss_weight");
469562
}
470-
471-
// OpenAI migration guard at record level
472-
if ("name" in record && !("messages" in record)) {
473-
// Only warn at record level if it's not inside messages (messages handled above)
474-
out.push(
475-
makeIssue(
476-
"warning",
477-
"UNSUPPORTED_FIELD_NAME",
478-
`Record-level field "name" is not supported by Bailian.`,
479-
{ line: lineNo, path: "name" },
480-
),
481-
);
563+
for (let idx = 0; idx < messages.length; idx++) {
564+
const msg = messages[idx] as Record<string, unknown> | null;
565+
if (!msg || !("loss_weight" in msg)) continue;
566+
checkLossWeightRange(msg.loss_weight, `messages[${idx}].loss_weight`);
567+
if (!(msg.role === "assistant" && idx === lastAssistantIdx)) {
568+
out.push(
569+
makeIssue(
570+
"warning",
571+
"LOSS_WEIGHT_PLACEMENT",
572+
`"loss_weight" is only supported on the last assistant message; found at messages[${idx}] (role "${String(msg.role)}").`,
573+
{ line: lineNo, path: `messages[${idx}].loss_weight` },
574+
),
575+
);
576+
}
482577
}
578+
579+
// OpenAI migration guard at record level (message-level occurrences are
580+
// handled by inspectMessageObject above)
483581
if ("weight" in record) {
484582
out.push(
485583
makeIssue(
486-
"warning",
584+
"error",
487585
"UNSUPPORTED_FIELD_WEIGHT",
488-
`Record-level field "weight" is not supported by Bailian. Remove it when migrating from OpenAI/Azure.`,
586+
`Record-level field "weight" is not supported by Bailian and must be removed when migrating from OpenAI/Azure.`,
489587
{ line: lineNo, path: "weight" },
490588
),
491589
);

packages/core/src/dataset/validate/schemas/dpo.ts

Lines changed: 63 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,16 +18,70 @@ function inspectDPORecord(record: Record<string, unknown>, lineNo: number): Vali
1818
const messages = record.messages;
1919
if (!Array.isArray(messages) || messages.length === 0) return out;
2020

21-
// DPO convention: messages should end with a user message (the prompt that
22-
// chosen/rejected respond to). If the last message is assistant, it's likely
23-
// a structural mistake.
21+
/** image / video content items are outside the DPO support matrix */
22+
const mediaIssues = (content: unknown, basePath: string): ValidationIssue[] => {
23+
if (!Array.isArray(content)) return [];
24+
const found: ValidationIssue[] = [];
25+
for (let itemIdx = 0; itemIdx < content.length; itemIdx++) {
26+
const item = content[itemIdx] as Record<string, unknown> | null;
27+
if (!item || typeof item !== "object") continue;
28+
for (const mediaField of ["image", "video"] as const) {
29+
if (mediaField in item) {
30+
found.push(
31+
makeIssue(
32+
"error",
33+
"DPO_UNSUPPORTED_ELEMENT",
34+
`DPO training data does not support ${mediaField} inputs; found at ${basePath}.content[${itemIdx}].`,
35+
{ line: lineNo, path: `${basePath}.content[${itemIdx}].${mediaField}` },
36+
),
37+
);
38+
}
39+
}
40+
}
41+
return found;
42+
};
43+
44+
// Support matrix (platform spec): DPO is text + thinking ONLY — no image /
45+
// video inputs and no tool calling. Reject multimodal items and tool fields
46+
// that the SFT-oriented ChatML inspector would otherwise accept.
47+
if ("tools" in record) {
48+
out.push(
49+
makeIssue(
50+
"error",
51+
"DPO_UNSUPPORTED_ELEMENT",
52+
`DPO training data does not support tool calling; remove the "tools" definition.`,
53+
{ line: lineNo, path: "tools" },
54+
),
55+
);
56+
}
57+
for (let idx = 0; idx < messages.length; idx++) {
58+
const msg = messages[idx] as Record<string, unknown> | null;
59+
if (!msg) continue;
60+
const msgPath = `messages[${idx}]`;
61+
if (msg.role === "tool" || "tool_calls" in msg) {
62+
out.push(
63+
makeIssue(
64+
"error",
65+
"DPO_UNSUPPORTED_ELEMENT",
66+
`DPO training data does not support tool calling; found ${
67+
msg.role === "tool" ? `role "tool"` : `"tool_calls"`
68+
} at ${msgPath}.`,
69+
{ line: lineNo, path: msgPath },
70+
),
71+
);
72+
}
73+
out.push(...mediaIssues(msg.content, msgPath));
74+
}
75+
76+
// DPO trains the preference for the LAST user input — messages ending with
77+
// any other role make the chosen/rejected pair semantically meaningless.
2478
const lastMsg = messages[messages.length - 1] as Record<string, unknown> | null;
2579
if (lastMsg && lastMsg.role !== "user") {
2680
out.push(
2781
makeIssue(
28-
"warning",
82+
"error",
2983
"DPO_LAST_MSG_NOT_USER",
30-
`DPO "messages" should end with a "user" message (the prompt for chosen/rejected). ` +
84+
`DPO "messages" must end with a "user" message (the prompt for chosen/rejected). ` +
3185
`Got "${String(lastMsg.role)}" as the last message.`,
3286
{ line: lineNo, path: `messages[${messages.length - 1}].role` },
3387
),
@@ -55,6 +109,7 @@ function inspectDPORecord(record: Record<string, unknown>, lineNo: number): Vali
55109
}
56110
if (hasChosen) {
57111
out.push(...inspectMessageObject(record.chosen, lineNo, "chosen"));
112+
out.push(...mediaIssues((record.chosen as Record<string, unknown> | null)?.content, "chosen"));
58113
const role = (record.chosen as Record<string, unknown> | null)?.role;
59114
if (typeof role === "string" && role !== "assistant") {
60115
out.push(
@@ -69,6 +124,9 @@ function inspectDPORecord(record: Record<string, unknown>, lineNo: number): Vali
69124
}
70125
if (hasRejected) {
71126
out.push(...inspectMessageObject(record.rejected, lineNo, "rejected"));
127+
out.push(
128+
...mediaIssues((record.rejected as Record<string, unknown> | null)?.content, "rejected"),
129+
);
72130
const role = (record.rejected as Record<string, unknown> | null)?.role;
73131
if (typeof role === "string" && role !== "assistant") {
74132
out.push(

0 commit comments

Comments
 (0)