Repository navigation
Conversation
Signed-off-by: 0z5a <0z5a@users.noreply.github.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a version and token alignment contract for collect-only draft features, defining data schemas and validation rules for sequences, tokens, and feature manifests, along with corresponding unit tests and documentation. The feedback highlights a potential type mismatch bug when validating sequence offsets against deserialized lists, and suggests improving validation robustness by providing more specific error messages and enforcing strict ordering of selected tokens by their packed positions.
| offsets = [0] | ||
| for sequence in self.sequences: | ||
| offsets.append(offsets[-1] + len(sequence.token_ids)) | ||
| if tuple(offsets) != self.sequence_offsets or not self.selected_tokens: |
There was a problem hiding this comment.
Comparing a tuple directly to a list (e.g., tuple(offsets) != self.sequence_offsets) will always evaluate to True in Python because their types differ. Since sequence_offsets is often deserialized as a list from JSON payloads, this direct comparison will cause valid token maps to fail validation. Converting self.sequence_offsets to a tuple before comparison resolves this issue.
| if tuple(offsets) != self.sequence_offsets or not self.selected_tokens: | |
| if tuple(offsets) != tuple(self.sequence_offsets) or not self.selected_tokens: |
| for token in self.selected_tokens: | ||
| if token.sample_id not in samples: | ||
| raise ValueError("selected token references an unknown sample") | ||
| sequence, start = samples[token.sample_id] | ||
| position = token.token_position | ||
| if position < 0 or position >= len(sequence.token_ids) - 1 or sequence.loss_mask[position] != 1: | ||
| raise ValueError("selected token does not match its source sequence") | ||
| if token.packed_position != start + position or token.target_token_id != sequence.token_ids[position + 1]: | ||
| raise ValueError("selected token does not match its source sequence") | ||
| if (token.sample_id, position) in selected: | ||
| raise ValueError("selected token does not match its source sequence") | ||
| selected.add((token.sample_id, position)) |
There was a problem hiding this comment.
This block has two opportunities for improvement:\n1. Specific Error Messages: Multiple distinct validation checks raise the exact same generic error message: "selected token does not match its source sequence". This makes debugging extremely difficult. Providing more specific error messages for each failure mode (while retaining the substring "source sequence" to keep existing tests passing) greatly improves maintainability.\n2. Enforce Ordering: There is currently no check to ensure that selected_tokens are strictly ordered by their packed_position. Since downstream consumers likely expect ordered tokens for sequential indexing/slicing, we should enforce that token.packed_position is strictly increasing.
last_packed = -1\n for token in self.selected_tokens:\n if token.sample_id not in samples:\n raise ValueError(f"selected token references an unknown sample_id: {token.sample_id}")\n sequence, start = samples[token.sample_id]\n position = token.token_position\n if position < 0 or position >= len(sequence.token_ids) - 1 or sequence.loss_mask[position] != 1:\n raise ValueError(f"selected token position {position} does not match its source sequence mask or bounds")\n if token.packed_position != start + position or token.target_token_id != sequence.token_ids[position + 1]:\n raise ValueError(f"selected token packed position or target does not match its source sequence")\n if (token.sample_id, position) in selected:\n raise ValueError(f"duplicate selected token does not match its source sequence uniqueness constraint")\n if token.packed_position <= last_packed:\n raise ValueError("selected tokens must be strictly ordered by packed position")\n selected.add((token.sample_id, position))\n last_packed = token.packed_positionSigned-off-by: 0z5a <0z5a@users.noreply.github.com>
Signed-off-by: 0z5a <dezhen.lu@student.uni-tuebingen.de>
|
Superseded by #454, now merged into |
Incremental review: Files changed.
Base:
0z5a-branchat791f101e20db377b28e15f12a00863baaea2605f.Define a versioned, collect-only external-draft feature manifest and causal token map. The schema records target/head versions, model/tokenizer identity and readiness so a consumer can reject incomplete or incompatible batches.
The collector and its real-model validation are in #454. This layer defines the schema; draft optimization and weight publication require further implementation. Test harnesses and audit reports were removed from the submitted diff.