[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.94521% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.39%. Comparing base (cf293a7) to head (a3eac3d).

Files with missing lines	Patch %	Lines
...hine/translation/llm/coverage_example_retriever.py	95.89%	3 Missing ⚠️
machine/translation/llm/__init__.py	90.90%	1 Missing ⚠️
machine/translation/llm/example_retriever.py	94.11%	1 Missing ⚠️
machine/translation/llm/top_k_example_retriever.py	95.23%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #412      +/-   ##
==========================================
+ Coverage   92.32%   92.39%   +0.06%     
==========================================
  Files         399      412      +13     
  Lines       25541    25833     +292     
==========================================
+ Hits        23582    23868     +286     
- Misses       1959     1965       +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]

F14 still applies at e24c4e2, and now in two places. The anchors still come from similarities.argsort()[::-1] at coverage_example_retriever.py:47, and the new k >= n shortcut at line 49 returns that same order. Coverage([same/0, same/1]).rank("same", 2) returns [same/1, same/0], while TF-IDF returns [same/0, same/1]. The k=2 case over four rows still returns [same/1, same/2]. np.argsort(-similarities, kind="stable") would fix both (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).

[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.

[benjaminking]

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

---

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

Previously, Enkidu93 (Eli C. Lowry) wrote…

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

I think creating list of unique examples should work. Once you switch back to excluding only a single example, you can check whether the example being excluded is unique, and if it is, exclude it from the retrieved examples, and otherwise include it.

---

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

Previously, Enkidu93 (Eli C. Lowry) wrote…

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.

Thanks, I answered it on the next question thread.

---

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

Previously, mshannon-sil wrote…

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.

The reason we had the functionality for excluding examples was because this class was shared between local LLMs and remote LLMs. And when we fine-tune local LLMs, we need to have the ability to exclude a single example for fine-tuning, while still allowing identical examples from elsewhere in the corpus into the set of few shot examples.

So, it should be fine to revert to excluding a single example. Matthew's solution would also work, but I'd prefer if the example retriever could be shared between local and remote LLMs in SILNLP.

---

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

Previously, mshannon-sil wrote…

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.

Damien's initial implementation of "whole corpus in the prompt" didn't order the examples, but it does probably make sense to allow for ordering.

[mshannon-sil]

@Enkidu93 I'm realizing that we'll also need the ability to retrieve the surrounding context for a verse as well. For example, in the post editing, I'm putting a 2 verse window in the context in addition to the retrieved examples. Up to you if you think this should go in this PR or if I should just add that functionality with the post editing. I do think it'll be applicable for both translating and post editing.

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

[Enkidu93]

I think it'd be better to inject that separately.

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

---

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

Previously, benjaminking (Ben King) wrote…

I think creating list of unique examples should work. Once you switch back to excluding only a single example, you can check whether the example being excluded is unique, and if it is, exclude it from the retrieved examples, and otherwise include it.

Done.

---

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

Previously, benjaminking (Ben King) wrote…

Thanks, I answered it on the next question thread.

Done. Thank you!

---

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

Previously, benjaminking (Ben King) wrote…

The reason we had the functionality for excluding examples was because this class was shared between local LLMs and remote LLMs. And when we fine-tune local LLMs, we need to have the ability to exclude a single example for fine-tuning, while still allowing identical examples from elsewhere in the corpus into the set of few shot examples.

So, it should be fine to revert to excluding a single example. Matthew's solution would also work, but I'd prefer if the example retriever could be shared between local and remote LLMs in SILNLP.

OK, that makes more sense. Done. Let me know if you see any issues.

---

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

Previously, benjaminking (Ben King) wrote…

Damien's initial implementation of "whole corpus in the prompt" didn't order the examples, but it does probably make sense to allow for ordering.

Yep, done. Claude brought that up as well. I guess the method is called rank so seems appropriate ( - originally, it was called select). I'm not sure why you would ask for more than you want, but even for the sake of avoiding forgetting due to the serial position effect (which I think also affects LLMs), it's probably worth updating.

[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:47. When rows tie, Coverage favours later rows, both in the anchors it picks and now in its new k >= n shortcut, while TF-IDF favours earlier ones.
3. Counts: 0 Critical, 0 Important, 2 Low still open (F14, F15). No new findings this round.
4. What I ran:
   • The compare against 2c99125 returned ahead. The one new commit, e24c4e2, changes exclude_example back to exclude_index (only that index is dropped, and an out-of-range index is ignored), moves the k >= n shortcut into Coverage, and adds an examples property.
   • uv run pytest tests/translation/llm: 25 passed.
   • A repro against head:
     • F10 is fixed: TF-IDF and Coverage both return [apple pie, x y] for k = n.
     • F14 still reproduces.
     • Coverage's exclude-by-index logic does what its tests say for [A, A, B], [A, B, A] and [A, A, A, B], for every exclude index and for k = 1..3.
   • 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. Also 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). Since the last round, rank's third parameter changed from exclude_example: Optional[Example] to exclude_index: Optional[int], and ExampleRetriever.examples was added. Neither has been released yet. There is no sillsdev/machine parity contract (None verified).

Status of every finding:

Finding	Status
F1–F9, F11–F13, F16–F18, F21	addressed (earlier rounds)
F10	addressed in e24c4e2 (it had been accepted)
F19	accepted: Coverage's k >= n shortcut still keeps duplicate pairs
F20	withdrawn
F14	open; it now also affects the new Coverage shortcut
F15	open

Reviewed at e24c4e2

[benjaminking]

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

---

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

Previously, Enkidu93 (Eli C. Lowry) wrote…

OK, that makes more sense. Done. Let me know if you see any issues.

I'm not sure I understand the logic around an example being the first occurrence of a multiply attested example. Can you explain that? I would think examples would be treated the same regardless of the order they appear in.

---

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

Previously, Enkidu93 (Eli C. Lowry) wrote…

Yep, done. Claude brought that up as well. I guess the method is called rank so seems appropriate ( - originally, it was called select). I'm not sure why you would ask for more than you want, but even for the sake of avoiding forgetting due to the serial position effect (which I think also affects LLMs), it's probably worth updating.

I might not be understanding it correctly, but it doesn't look like this addresses the problem of k being greater than the number of examples. It now gets routed to _rank(), but _rank() still returns without ordering the examples if k is too large.

---

machine/translation/llm/example_retriever.py line 17 at r5 (raw file):

    @property
    def examples(self) -> List[Example]:

This is minor, but I like to make a rule of not adding to a class's interface for the purposes of testing, and just have the tests cover the existing exposed methods, splitting into smaller classes when it becomes difficult to test all the functionality that way.

[Enkidu93]

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

---

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

Previously, benjaminking (Ben King) wrote…

I'm not sure I understand the logic around an example being the first occurrence of a multiply attested example. Can you explain that? I would think examples would be treated the same regardless of the order they appear in.

Yeah, sorry, this is a little convoluted. If you see a simpler way that doesn't involve lots of duplication, please let me know.

Here's an example. If we have examples [a, b, a, c] and we exclude the example at index 0, we want to still include the example at index 2 and if we exclude the example at 2, we still want to include the example at index 0. However, because the coverage retriever deduplicates the examples in its normal ranking, this requires a little extra care.

So if examples = [a, b, a, c], the deduplicated indices would be [0, 1, 3] and the unique indices would be [1,3].

So this line

  if exclude_index is not None and (
            exclude_index not in self._deduplicated_example_indices or exclude_index in self._unique_example_indices
        ):

translates to: If you have selected an index to be excluded and it is not one of the deduplicated indices, there's no point in excluding it since it'll get stripped anyways (this part could just be removed I guess - it was mainly for clarity but apparently it wasn't so clear haha). But if it is one of the deduplicated indices, we should only exclude it if it is unique. If it is not unique, then we'll just retain that example in lieu of its duplicate elsewhere in the examples. The 'first occurence' phrase is just referring to the way that the deduplicated indices list is organized - i.e., in our example, it is [0, 1, 3], not [1, 2, 3].

Does that make sense? I could just be being stupid, so please push back. This was my attempt to do what you suggested regarding adding back in a duplicate example.

---

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

Previously, benjaminking (Ben King) wrote…

I might not be understanding it correctly, but it doesn't look like this addresses the problem of k being greater than the number of examples. It now gets routed to _rank(), but _rank() still returns without ordering the examples if k is too large.

Which implementation of _rank() are you seeing that doesn't return them in order? I had added a line in the coverage retriever to return them early in ranked order:

        if k >= ranked.shape[0]:
            return [self._examples[i] for i in ranked if i != exclude_index]

I think it'd work without that line, but just to save a few cycles...

And in the top-k retriever, it should return them in ranked order, right?

        ...
        top = np.argpartition(-scores, k - 1)[:k] if k < n else np.arange(n)
        top_indices: List[int] = top[np.argsort(-scores[top])].tolist()
        return [self._examples[i] for i in top_indices]

I'm not sure there's a way to really return much earlier here.

What am I missing?

---

machine/translation/llm/example_retriever.py line 17 at r5 (raw file):

Previously, benjaminking (Ben King) wrote…

This is minor, but I like to make a rule of not adding to a class's interface for the purposes of testing, and just have the tests cover the existing exposed methods, splitting into smaller classes when it becomes difficult to test all the functionality that way.

I agree - I actually did not add this for testing. I added it because I didn't like the idea of having the ability to exclude only by index in rank() without the client having the ability to see the retriever's ordered examples. It's not absolutely necessary though of course. I'm happy to keep it or to strip it out.

[benjaminking]

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

---

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

Previously, Enkidu93 (Eli C. Lowry) wrote…

Yeah, sorry, this is a little convoluted. If you see a simpler way that doesn't involve lots of duplication, please let me know.

Here's an example. If we have examples [a, b, a, c] and we exclude the example at index 0, we want to still include the example at index 2 and if we exclude the example at 2, we still want to include the example at index 0. However, because the coverage retriever deduplicates the examples in its normal ranking, this requires a little extra care.

So if examples = [a, b, a, c], the deduplicated indices would be [0, 1, 3] and the unique indices would be [1,3].

So this line

  if exclude_index is not None and (
            exclude_index not in self._deduplicated_example_indices or exclude_index in self._unique_example_indices
        ):

translates to: If you have selected an index to be excluded and it is not one of the deduplicated indices, there's no point in excluding it since it'll get stripped anyways (this part could just be removed I guess - it was mainly for clarity but apparently it wasn't so clear haha). But if it is one of the deduplicated indices, we should only exclude it if it is unique. If it is not unique, then we'll just retain that example in lieu of its duplicate elsewhere in the examples. The 'first occurence' phrase is just referring to the way that the deduplicated indices list is organized - i.e., in our example, it is [0, 1, 3], not [1, 2, 3].

Does that make sense? I could just be being stupid, so please push back. This was my attempt to do what you suggested regarding adding back in a duplicate example.

I don't think we actually want to exclude differently depending on which a we're looking at. In both cases, we should not exclude the duplicate a.

Since the use case for exclusion is fine-tuning, I think we want to include every instance other than the one we're fine-tuning on (because we want to train the model to copy when an identical verse is present). So, we should be able to have the logic be as simple as "only exclude the example at the index specified, regardless of whether there are duplicates."

---

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

Previously, Enkidu93 (Eli C. Lowry) wrote…

Which implementation of _rank() are you seeing that doesn't return them in order? I had added a line in the coverage retriever to return them early in ranked order:

        if k >= ranked.shape[0]:
            return [self._examples[i] for i in ranked if i != exclude_index]

I think it'd work without that line, but just to save a few cycles...

And in the top-k retriever, it should return them in ranked order, right?

        ...
        top = np.argpartition(-scores, k - 1)[:k] if k < n else np.arange(n)
        top_indices: List[int] = top[np.argsort(-scores[top])].tolist()
        return [self._examples[i] for i in top_indices]

I'm not sure there's a way to really return much earlier here.

What am I missing?

Ah, ok. My mistake. Sorry.

---

machine/translation/llm/example_retriever.py line 17 at r5 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

I agree - I actually did not add this for testing. I added it because I didn't like the idea of having the ability to exclude only by index in rank() without the client having the ability to see the retriever's ordered examples. It's not absolutely necessary though of course. I'm happy to keep it or to strip it out.

Ok, this should be good as is then.

[benjaminking]

@benjaminking made 2 comments and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on ddaspit and pmachapman).

---

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

Previously, benjaminking (Ben King) wrote…

I don't think we actually want to exclude differently depending on which a we're looking at. In both cases, we should not exclude the duplicate a.

Since the use case for exclusion is fine-tuning, I think we want to include every instance other than the one we're fine-tuning on (because we want to train the model to copy when an identical verse is present). So, we should be able to have the logic be as simple as "only exclude the example at the index specified, regardless of whether there are duplicates."

Eli and I discussed this offline, and I'm ok with this as implemented.

[pmachapman]

@pmachapman partially reviewed 18 files and all commit messages, and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on ddaspit).