SEZ9 commented on PR #12241:
URL: https://github.com/apache/seatunnel/pull/12241#issuecomment-5621371866
Thanks for picking this up, @goutamadwant. I wrote the original harness in
#11553, so this lands squarely in an area I care about — and it addresses a
real structural weakness rather than adding surface area.
To make the value explicit for other reviewers: the 100-task corpus has
fixed, publicly visible wording. The improvement roadmap behind #11616 is
mostly prompt and metadata work (connector option injection, golden examples
for conditional routing, CDC prerequisites, wide-DAG wiring rules). Without a
paraphrase suite, a score increase from that work cannot be separated from the
model having memorized the exact task phrasing. This suite gives that A/B a
second dimension. Notably the variant distribution lines up with the measured
all-model failure clusters — 4 of 12 are t3_cr_* conditional routing and 4 are
t2_cdc_* — which is where the ambiguity between "learned the wiring rule" and
"matched the wording" matters most.
What I like
load_paraphrases fails closed everywhere it can: the strict key whitelist
(set(variant) != {"parent_id", "parent_sha256", "prompt"}), unknown parent,
empty/malformed corpus, and a variant whose prompt equals its parent's all
raise rather than degrade.
copy.deepcopy(parent) with
test_variants_inherit_the_complete_task_without_aliasing guarding it.
Shallow-copying here would have silently shared assertion lists between parent
and variant.
The parent fingerprint uses the same canonicalization as task_fingerprints
in run_benchmark (sort_keys=True, separators=(",", ":")), so the two hashing
schemes stay consistent.
Moving load_tasks ahead of build_models_from_args so an invalid selection
fails before provider setup, i.e. before any billable call.
The docs are honest in the places that are easy to oversell: "not an unseen
holdout", "this command calls a model, even with --level l1", and the explicit
warning not to compare parent IDs against variant IDs or combine their rates as
independent evidence. That last line is exactly the misuse I would have worried
about.
Suggestions
Record the suite in the run metadata. all_results in run_benchmark carries
levels, max_repairs, trials, cli and models, and this PR adds
parent_id/parent_sha256 per task entry — but there is no run-level suite
marker, so a results file is not identifiable as a paraphrase run without
inspecting task entries. I checked compare.py and this is not a correctness
problem: a cross-suite comparison hits excluded(..., f"missing {side} task")
for every task, so no bogus delta is produced. But the operator gets 12–100
exclusion rows instead of one clear error. Adding all_results["suite"] = suite
plus a check in _run_issues would enforce in code what the docs currently only
advise.
One variant per parent is a hard ceiling. task_id = parent_id + "_p1" means
a second wording for the same parent raises Duplicate paraphrase task ID, and
test_identical_prompt_and_duplicate_parent_are_rejected shows that is
deliberate rather than an oversight. Was an index-based scheme (_p1, _p2, …)
considered, or is the ceiling intentionally left for a follow-up? Conditional
routing is the highest-failure cluster, and it is the one where two or three
phrasings would be most informative.
Repinning has no tooling. The 12 parent_sha256 values are maintained by
hand. One planned item on the roadmap is adding semantic verification probes to
the ~96 tasks that lack them; that single change invalidates all 12 pins at
once and requires recomputing each hash manually. A small --repin mode on
benchmark.paraphrases that recomputes and writes the fingerprints back would
keep the maintenance cost from making this suite rot. The review gate stays
intact as long as repinning is an explicit, reviewable diff.
Nits, take or leave
The task_ids validation (non-empty, distinct, known) exists only on the
paraphrase path. On baseline, --tasks does-not-exist still silently yields
nothing and reports "No tasks selected." Hoisting the check would benefit both.
any(t not in TIER_FILES for t in tiers) is unreachable from the CLI, since
argparse's choices=[1, 2, 3] already validates each element of --tiers; only
the duplicate check is new there.
A variant's own task_sha256 includes parent_sha256, so rewording a parent's
prompt alone forces a repin, which changes the variant's fingerprint, which
makes compare.py exclude it as a changed contract — even though the variant's
own prompt, assertions and fixtures are unchanged. Narrow case, low priority;
excluding the provenance keys from the variant's fingerprint would avoid it.
One thing that is on me, not on this PR
These tests will not run in CI. setup-python at
.github/workflows/backend.yml:160 only feeds the change-detection script, no
workflow invokes pytest, and the benchmarks path filter matches
seatunnel-benchmarks/** and tools/benchmarks/** rather than
seatunnel-cli/benchmark/**. So the 52 new offline cases here, and the ~181
pre-existing CLI tests, are verified only on the contributor's machine. That
gap came in with #11553 and is mine to fix — I will open a separate issue to
wire seatunnel-cli tests into CI. Please do not expand this PR's scope for it;
I am noting it so the "how was this tested" section is read correctly, since
the fork Build passing does not cover any of this.
Overall this is a well-scoped, well-tested addition and I do not see a
blocker. Happy to approve once the suite marker in (1) is addressed, and (2)
answered either way.
Two notes for you before posting:
I wrote (3) as a suggestion rather than a request, since it's arguably
follow-up work. Tell me if you'd rather drop it entirely to keep the review
tight.
The "one thing that is on me" paragraph commits you to filing the CI issue.
Say the word and I'll draft that issue + PR — it doesn't conflict with the
three Zeta branches, so it can run in parallel.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
