[Enkidu93]

Resolves #399.

This does not include all retrieval methods in silnlp; only those that we will currently need for production. I've intentionally diverged in a few places from silnlp but do please leave comments if you seem gaps in capabilities or if you'd prefer silnlp's API/architecture. The tests are mostly ports of silnlp tests so the functionality ought to be the same.

(Still out traveling but had a couple minutes so thought I'd put this in since I didn't get to it before I left).

---

This change is 

[codecov-commenter]

Codecov Report

❌ Patch coverage is 97.82609% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.33%. Comparing base (abe3718) to head (2c99125).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...hine/translation/llm/coverage_example_retriever.py	97.01%	2 Missing ⚠️
machine/translation/llm/__init__.py	90.90%	1 Missing ⚠️
machine/translation/llm/example_retriever.py	93.33%	1 Missing ⚠️
machine/translation/llm/top_k_example_retriever.py	95.45%	1 Missing ⚠️
tests/testutils/mock_example_retriever.py	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #412      +/-   ##
==========================================
+ Coverage   92.27%   92.33%   +0.05%     
==========================================
  Files         398      411      +13     
  Lines       25461    25737     +276     
==========================================
+ Hits        23494    23764     +270     
- Misses       1967     1973       +6     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Enkidu93]

@claude Please review this PR.

[claude]

FYI: F14. reverse=True on (similarity, i), together with argsort()[::-1] at line 39, breaks ties toward later rows. Repro: [same/0, same/1, same/2, other/3] with rank("same", 2) returns [same/2, same/1]. That is deterministic, but the comment does not say later rows win, which is the opposite of the usual tie-break.

[Enkidu93]

Done. Switched to -i.

[claude]

F14 addressed in db62062: the sort key is now (similarities[i], -i) with reverse=True, so ties go to earlier rows.

[claude]

F14 still applies at c1356ad. This corrects my reply above. The final sort now orders ties toward earlier rows, but the rows that get chosen still favour later ones, because the anchors come from similarities.argsort()[::-1] (line 42). Coverage([same/0, same/1, same/2, other/3]).rank("same", 2) returns [same/1, same/2], while TF-IDF returns [same/0, same/1]. A stable descending sort, such as np.argsort(-similarities, kind="stable"), should fix the selection (untested).

[claude]

FYI: F15. The _vectorizer/_matrix state, the empty-corpus guard, and the None checks repeat TfidfExampleRetriever. The two guards already disagree (strip() against _tokenize). A shared TF-IDF base or helper would keep them in step.

[benjaminking]

@benjaminking reviewed 14 files and all commit messages, and made 5 comments.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on ddaspit, Enkidu93, mshannon-sil, and pmachapman).

---

machine/translation/llm/coverage_example_retriever.py line 13 at r1 (raw file):

class CoverageExampleRetriever(LazyExampleRetriever):
    # Benchmarked as a set; the measured gain says nothing about other values.

You can probably remove some of these Claude comments.

---

machine/translation/llm/coverage_example_retriever.py line 47 at r1 (raw file):

        if vector.nnz == 0:
            return [self.examples[i] for i in ranked[:k].tolist()]
        candidates = self._distinct_candidates(

A totally minor thing, but I wonder if it would be better to create the list of distinct candidates upon creation of the example retriever object, since it is shared for all queries. I guess it gets a little complicated trying to exclude an example, as you'd have to know whether there were multiple exact matches in the full list of examples or not.

---

machine/translation/llm/coverage_example_retriever.py line 75 at r1 (raw file):

        return np.asarray(candidates)

    def _choose_for_coverage(self, vector, candidate_matrix, similarities: np.ndarray, k: int) -> List[int]:

Nit: two of the arguments are missing type hints

---

machine/translation/llm/coverage_example_retriever.py line 84 at r1 (raw file):

        available = np.ones(len(features), dtype=bool)
        available[chosen] = False
        counts = features[chosen].sum(axis=0).astype(float)

I'm fully aware that this is based on code that I "contributed" to SILNLP (Claude actually wrote it), but I think it would be worth taking a quick pass at adding some clarity to this function, since this is the crux of the coverage retriever. It's pretty tough to understand currently.

---

machine/translation/llm/lazy_example_retriever.py line 23 at r1 (raw file):

    @abstractmethod
    def _fit(self) -> None: ...

In SILNLP, the example retriever was created before any examples were available, so fitting the index had to be a separate step. But in Machine.py, we pass the examples to the constructor. So I think you can just call _fit() from the constructor here. I think this would simplify things significantly, as you wouldn't need to track whether it was fitted, and I think you could avoid needing locks.

[Enkidu93]

@Enkidu93 made 15 comments and resolved 13 discussions.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on benjaminking, ddaspit, mshannon-sil, and pmachapman).

---

machine/translation/llm/coverage_example_retriever.py line 13 at r1 (raw file):

Previously, benjaminking (Ben King) wrote…

You can probably remove some of these Claude comments.

Done. I was trying to do that, but I missed this one.

---

machine/translation/llm/coverage_example_retriever.py line 47 at r1 (raw file):

Previously, benjaminking (Ben King) wrote…

A totally minor thing, but I wonder if it would be better to create the list of distinct candidates upon creation of the example retriever object, since it is shared for all queries. I guess it gets a little complicated trying to exclude an example, as you'd have to know whether there were multiple exact matches in the full list of examples or not.

Done. I think I've handled this properly, but let me know if it looks off.

---

machine/translation/llm/coverage_example_retriever.py line 75 at r1 (raw file):

Previously, benjaminking (Ben King) wrote…

Nit: two of the arguments are missing type hints

Done.

---

machine/translation/llm/coverage_example_retriever.py line 84 at r1 (raw file):

Previously, benjaminking (Ben King) wrote…

I'm fully aware that this is based on code that I "contributed" to SILNLP (Claude actually wrote it), but I think it would be worth taking a quick pass at adding some clarity to this function, since this is the crux of the coverage retriever. It's pretty tough to understand currently.

Haha, I've added some human comments. Let me know if it's easier to understand now & if the comments seem accurate.

---

machine/translation/llm/lazy_example_retriever.py line 23 at r1 (raw file):

Previously, benjaminking (Ben King) wrote…

In SILNLP, the example retriever was created before any examples were available, so fitting the index had to be a separate step. But in Machine.py, we pass the examples to the constructor. So I think you can just call _fit() from the constructor here. I think this would simplify things significantly, as you wouldn't need to track whether it was fitted, and I think you could avoid needing locks.

After some discussion outside of this thread, I have removed the lazy fitting since I misunderstood the situation in silnlp and thought that this was a requirement.

[Enkidu93]

Done. Switched to -i.

[Enkidu93]

@Enkidu93 made 6 comments and resolved 2 discussions.
Reviewable status: 4 of 18 files reviewed, 5 unresolved discussions (waiting on benjaminking, ddaspit, mshannon-sil, and pmachapman).

[claude]

1. Verdict: approve.
2. Most important: nothing blocking is left. The most useful open item is F14 at machine/translation/llm/coverage_example_retriever.py:42. When rows tie, Coverage's anchors favour later rows (argsort()[::-1]), while TF-IDF favours earlier ones.
3. Counts: 0 Critical, 0 Important, 2 Low still open (F14, F15). Nothing new this round.
4. What I ran:
   • The compare against b3be109 returned ahead. The diff since then is the one-line F18 fix in top_k_example_retriever.py and the F21 file rename. Because the change was that small, I re-checked the open findings and did not run a fresh /code-review.
   • TfidfExampleRetriever([apple pie, b, c])._rank("apple pie", 1) returns [apple pie], so F18 is fixed.
   • uv run pytest tests/translation/llm: 24 passed.
   • gh pr checks 412: all Build jobs, claude-review, and Reviewable are pending; comment hygiene passed.
5. Not verified:
   • CI lint, type, and test results, which are still pending.
   • The stable-sort fix suggested for F14.

API surface: this PR adds a new public package, machine.translation.llm, which ships in the published wheel, and a new optional extra, llm (scikit-learn). Nothing changed here since the last round. There is no sillsdev/machine parity contract (None verified).

Status of every finding:

Finding	Status
F1–F9, F11–F13, F16, F17	addressed (earlier rounds)
F18, F21	addressed in 2c99125
F10, F19	accepted: the author keeps the k >= n shortcut as designed
F20	withdrawn: TF-IDF stays plain top-k, and the caller can dedup
F14, F15	open

Reviewed at 2c99125

[benjaminking]

@benjaminking partially reviewed 14 files and all commit messages, made 3 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on ddaspit, Enkidu93, mshannon-sil, and pmachapman).

---

machine/translation/llm/coverage_example_retriever.py line 37 at r4 (raw file):

        self._matrix = cast(csr_matrix, self._vectorizer.fit_transform(sources))

    def _rank(self, query: str, k: int, exclude_indices: Optional[List[int]] = None) -> List[Example]:

What drove the change to exclude multiple indices?

---

machine/translation/llm/coverage_example_retriever.py line 45 at r4 (raw file):

        if exclude_indices is not None:
            ranked = ranked[np.isin(ranked, exclude_indices, invert=True)]
        ranked = ranked[np.isin(ranked, self._unique_example_indices)]

I don't think we should exclude everything that is not unique. If we have two identical source verses, we should still be able to include the first in the context for translating the second (and vice versa). Does that make sense?

[Enkidu93]

@Enkidu93 made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on benjaminking, ddaspit, mshannon-sil, and pmachapman).

---

machine/translation/llm/coverage_example_retriever.py line 37 at r4 (raw file):

Previously, benjaminking (Ben King) wrote…

What drove the change to exclude multiple indices?

Mainly because if you want to truly be able to exclude an Example, you need to be able to exclude all copies of that example. This is sort of connected to the following question.

---

machine/translation/llm/coverage_example_retriever.py line 45 at r4 (raw file):

Previously, benjaminking (Ben King) wrote…

I don't think we should exclude everything that is not unique. If we have two identical source verses, we should still be able to include the first in the context for translating the second (and vice versa). Does that make sense?

I agree in general, but in the context of the coverage retriever, do you think that's still true? Also, maybe I'm misunderstanding the case you're mentioning because I don't quite see how that could happen.

[mshannon-sil]

machine/translation/llm/coverage_example_retriever.py line 45 at r4 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

I agree in general, but in the context of the coverage retriever, do you think that's still true? Also, maybe I'm misunderstanding the case you're mentioning because I don't quite see how that could happen.

The case could occur if for example there's a verse in common across the gospels, or in a part of the Bible that quotes the previous part.

It sounds like the desire is not so much to remove anything that matches the test example, but just to not include the test example itself when judging the retriever in an experiment. But then it seems to me that use case could be addressed by not including any of the test examples/verses in the retrievers _examples in the first place, and we could eliminate all the exclude logic here.

[mshannon-sil]

@mshannon-sil partially reviewed 18 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on benjaminking, ddaspit, Enkidu93, and pmachapman).

---

machine/translation/llm/example_retriever.py line 25 at r4 (raw file):

            else []
        )
        if k >= (num_examples - len(exclude_indices)):

Minor: Should this go inside self._rank? I could see a class ordering all the examples, not to choose which ones, but just so that the most similar verses are put toward the end.