feat: refine — graded work items to training data with provenance (#14)#15
Open
OriNachum wants to merge 6 commits into
Open
feat: refine — graded work items to training data with provenance (#14)#15OriNachum wants to merge 6 commits into
OriNachum wants to merge 6 commits into
Conversation
…ady train/eval JSONL with provenance (#14) Co-Authored-By: Claude Fable 5 <[email protected]>
PR Summary by QodoAdd
AI Description
Diagram
High-Level Assessment
Files changed (12)
|
Code Review by Qodo
Context used✅ Tickets:
🎫 spec: refine — graded colleague work becomes training data with provenance [colleague#291/S6]✅ Compliance rules (platform):
25 rules✅ Skills:
version-bump, doc-test-alignment 1. Examples missing provenance metadata
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+173
to
+187
| def to_example(record: dict[str, Any]) -> dict[str, Any]: | ||
| """Map one kept record to the sloth **chat** schema. | ||
|
|
||
| Shape matches ``sloth.tune.datasets._validate_chat_record`` exactly: the | ||
| record has **only** the ``"messages"`` key (an extra key fails sloth's | ||
| ``extra = set(record.keys()) - CHAT_KEYS`` check), a non-empty list of | ||
| ``{"role", "content"}`` messages, roles ``"user"`` then ``"assistant"``. | ||
| """ | ||
| return { | ||
| "messages": [ | ||
| {"role": "user", "content": record["request"]}, | ||
| {"role": "assistant", "content": record["summary"]}, | ||
| ] | ||
| } | ||
|
|
There was a problem hiding this comment.
4. Examples missing provenance metadata 📎 Requirement gap ≡ Correctness
Emitted train.jsonl/eval.jsonl examples contain only messages and do not attach provenance using the Envelope metadata shape. This fails the requirement that each emitted example carry per-example provenance fields.
Agent Prompt
## Issue description
Each emitted training example must carry provenance fields (`source`, `task_id`, `rating`, `content_sha256`, `exported_at`) stored in/compatible with the Envelope `metadata` shape. The current `to_example()` output omits provenance entirely.
## Issue Context
Right now provenance is only written to `lineage.json`, but the compliance requirement is per-example provenance on the emitted examples themselves using the Envelope metadata shape.
## Fix Focus Areas
- data_refinery/refine/dataset.py[173-187]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Pushback — provenance is per-example, just not inline. Each surviving example's provenance (source, task_id, rating, content_sha256, exported_at) is written to lineage.json's examples[] array (docs/contract.md Wave 4, and asserted by test_three_line_fixture_provenance_shape). It cannot go inline in train.jsonl/eval.jsonl: sloth's chat-schema validator (_validate_chat_record) rejects any key beyond messages (extra = set(record.keys()) - CHAT_KEYS), so an inline provenance field would fail issue #14's own acceptance criterion that sloth train --dry-run accept the output verbatim. Co-locating provenance in the paired lineage file is the design, not a gap. Leaving open for your call.
- data-refinery-cli (Claude)
Comment on lines
+115
to
+127
| try: | ||
| text = path.read_text(encoding="utf-8") | ||
| except OSError as exc: | ||
| raise CliError( | ||
| code=EXIT_ENV_ERROR, | ||
| message=f"cannot read {path}: {exc.strerror or exc}", | ||
| remediation="check that the file exists and is readable", | ||
| ) from exc | ||
|
|
||
| valid: list[tuple[int, dict[str, Any]]] = [] | ||
| malformed: list[dict[str, Any]] = [] | ||
| for line_no, raw_line in enumerate(text.splitlines(), start=1): | ||
| line = raw_line.strip() |
There was a problem hiding this comment.
6. Jsonl not streamed 🐞 Bug ➹ Performance
parse_export_jsonl() reads the entire JSONL file into memory via read_text()+splitlines(), which contradicts the documented “JSONL stream” contract and can cause unnecessary memory spikes or MemoryError on large exports.
Agent Prompt
## Issue description
`parse_export_jsonl()` currently loads the full JSONL file into memory (`read_text()` then `splitlines()`). For large exports this creates avoidable memory pressure and undermines the stream-oriented contract language.
## Issue Context
The parser can be implemented in a streaming way using `path.open(...): for line_no, raw_line in enumerate(f, start=1)` while keeping the same diagnostics and counts.
## Fix Focus Areas
- data_refinery/refine/dataset.py[105-140]
- docs/contract.md[174-179]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Pushback (weak finding). The only occurrence of "stream" in contract.md describes the data shape — "a colleague feedback export JSONL stream (one graded work item per line)", i.e. one JSON object per line vs a single JSON array — not an I/O guarantee. Nothing in the contract promises incremental reads or bounded memory, so read_text() contradicts no documented behavior. Streaming is a reasonable optimization if exports ever grow large (Qodo's own summary listed it as a follow-up), but it is not a bug or contract violation for this PR. Can file a follow-up if you want it tracked. Leaving open.
- data-refinery-cli (Claude)
Triage of the Qodo review on PR #15 — three FIXes: - Finding 4 (Bug, High): parse_export_jsonl() and read_lineage() caught only OSError, so a non-UTF-8 --from/lineage.json raised an uncaught UnicodeDecodeError (a ValueError) that the dispatcher wrapped as exit 1 "unexpected: file a bug" instead of a structured EXIT_ENV_ERROR. Now caught alongside OSError at both read sites with an exit-2 "not valid UTF-8" hint. Adds two regression tests (parse + read_lineage). - SonarCloud gate (the actual merge-blocker): the "C Security Rating on New Code" is a single python:S2245 (weak PRNG) false positive on the seeded, deterministic train/eval shuffle — a non-cryptographic reproducible split. The existing `# nosec B311` is bandit-only; add `# NOSONAR` so SonarCloud stops flagging it and the New Code security rating returns to A. - Finding 1 (Skill insight): dropped the non-standard `### Notes` changelog heading (Keep-a-Changelog / rule 1217552 allows Added/Changed/Fixed); the stdlib-only note is folded into Added, and a Fixed entry documents the above. Pushed back (no code change; reasoning in the PR reply): #2 JSON diagnostics (repo-wide pattern, consistent with stack.py), #3 per-example provenance (it is in lineage.json — sloth's chat schema rejects extra keys), #5 JSONL streaming (contract's "stream" = data shape, not an I/O promise), #6 int|None handlers (matches 12/13 existing handlers). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01KM34gJRiKcuJii66Aw4xso
Contributor
Author
Triage of this review — 3 fixed, 3 pushed back, + the real CI blockerPushed Fixed:
Pushed back (reasoning in each inline thread, left open for you):
Still blocking merge: this branch is merge-conflicting with
|
Resolves the conflicts that appeared after #13 (fail-closed .gitignore) merged to main while this branch was open: - pyproject.toml: keep 0.10.0 (refine), ahead of main's 0.9.0 (gitignore). - CHANGELOG.md: keep both release sections — [0.10.0] refine (incl. the UTF-8 Fixed entry) above [0.9.0] gitignore from main. - docs/contract.md: keep both additions — main's `### Fail-closed .gitignore opt-in` stays a Wave-3 subsection; the new `## Wave 4 — refine` is its own top-level section before Versioning policy. - uv.lock: regenerated via `uv lock` (data-refinery-cli 0.10.0). Brings in #13's store work (files.py O_EXCL gitignore, migrate.py, test_store_*). Full suite green: 219 passed, 4 skipped; black/isort/flake8 clean; teken rubric 26/26. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01KM34gJRiKcuJii66Aw4xso
…properties The inline `# NOSONAR` added in fb6b9ce did not clear the finding — SonarCloud kept flagging `random.Random(seed).shuffle(order)` (split_items) and the New Code security rating stayed C, failing the gate. Move the waiver to a server-side `sonar.issue.ignore.multicriteria` rule (ruleKey python:S2245, resourceKey **/refine/dataset.py), which is honored reliably. It's a genuine false positive: the shuffle is a deterministic, seed-reproducible train/eval split — a non-cryptographic use of randomness. Inline comment trimmed to `# nosec B311` (bandit) with a pointer to the Sonar waiver. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01KM34gJRiKcuJii66Aw4xso
All maintainability/reliability smells flagged on PR #15 (none were gate- blocking); behavior unchanged, full suite green (219 passed). - S3776 (dataset.py, validate_export_line): cognitive complexity 21 -> under 15 by extracting _required_field_errors / _at_errors / _rating_errors / _notes_errors (mirroring the existing _stats_errors helper). Error strings and check order are byte-identical. - S7519 (dataset.py, _grade_distribution): {b: 0 for b in _RATING_BUCKETS} -> dict.fromkeys(_RATING_BUCKETS, 0). - S5799 (cli/_commands/refine.py): merged two implicitly-concatenated string literals in a remediation message into one. - S5958 x2 (tests/test_refine_dataset.py): pytest.raises(Exception) -> pytest.raises(CliError) for the parse_split_ratio failure tests (more specific, and a stronger assertion since parse_split_ratio raises CliError). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01KM34gJRiKcuJii66Aw4xso
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Lands
refine datasetandrefine lineage(issue #14, colleague#291 slice S6): a pure-function pipeline (data_refinery/refine/dataset.py) that turns acolleague feedback exportJSONL stream — one graded work item per line — into a sloth-ready chat-schema train/eval split plus a provenancelineage.json. Each stage is independently testable: shape-validate an export line (a malformed line is skipped, never silently dropped — one diagnostic per bad line, plus a counted total), filter tostatus == "ok"and a graded rating at/above--min-rating, map to the sloth chat schema, dedup same-content examples by reusing the existingfind_duplicate_groupssame-hash-same-scope primitive, and split train/eval via a deterministic seeded shuffle so the two sets are disjoint by construction. Every emitted example carries its per-example provenance (source task id, rating, exported-at timestamp).This closes the loop between colleague's ROI feedback store and unsloth-cli's fine-tuning surface: a graded work item can now become a training example without hand-rolled glue scripts.
docs/contract.mdgains Wave 4 documenting therefineconsumer contract (agent-first:--jsonon every verb, results to stdout/diagnostics to stderr, exit1on a missing--from/--datasetpath with ahint:), andexplain/overview/learngain matching catalog entries.Test plan
uv run pytest -n auto -q— 205 passed, 4 skipped (live-stack tests gated behindDATA_REFINERY_LIVE=1)uv run teken cli doctor . --strict— 26/26 checks passStdlib-only, no network, no LLM call throughout the new pipeline — no known deviations or honest limits beyond what's already documented in the shipped code/tests.
🤖 Generated with Claude Code