Repository navigation
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #412 +/- ##
==========================================
+ Coverage 92.27% 92.31% +0.04%
==========================================
Files 398 410 +12
Lines 25461 25764 +303
==========================================
+ Hits 23494 23784 +290
- Misses 1967 1980 +13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@claude Please review this PR. |
This comment has been minimized.
This comment has been minimized.
Remove example pool; add lock and tests; fix condition Update scikit version Explicitly cast Fix circular import Address Claude comments Fix formatting
62d42dc to
9125975
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| else (self.examples.index(exclude) if exclude in self.examples else None) | ||
| ) | ||
| if k >= num_examples: | ||
| return [example for index, example in enumerate(self.examples) if index != exclude_index] |
There was a problem hiding this comment.
Minor: F1. When k >= len(examples), this shortcut drops only the one excluded index and never reaches _distinct_candidates, so a copy of the excluded example leaks the answer into the prompt. Repro: CoverageExampleRetriever([apple pie/tarte, apple pie/tarte, banana]).rank("apple pie", 3, exclude=0) returns apple pie/tarte, but with k=1 it returns []. (Carried over from the earlier summary-only round; the ValueError half of F1 is fixed.)
| @@ -0,0 +1,17 @@ | |||
| from .coverage_example_retriever import CoverageExampleRetriever | |||
There was a problem hiding this comment.
Minor: F3. Without the llm extra, import machine.translation.llm still fails with a bare ModuleNotFoundError for sklearn. huggingface/__init__.py and thot/__init__.py guard this with is_*_available() and a RuntimeError that names the extra. Add the same guard to utils/packages. (Carried over from the earlier round.)
| if k == 0: | ||
| return [] | ||
| if vector.nnz == 0: | ||
| return [self.examples[i] for i in ranked[:k].tolist()] |
There was a problem hiding this comment.
Minor: F9. When the query shares no n-grams with the corpus, this returns ranked[:k] and skips _distinct_candidates, so a copy of the excluded row comes back. Repro: [apple pie/tarte, apple pie/tarte, banana split/b] with rank("zzzz", 2, exclude=0) returns [banana split, apple pie/tarte]. Run the dedup before this return.
| if isinstance(exclude, int) or exclude is None | ||
| else (self.examples.index(exclude) if exclude in self.examples else None) | ||
| ) | ||
| if k >= num_examples: |
There was a problem hiding this comment.
Minor: F10. When k >= n, results come back in corpus order, not ranked order, so whether the best match comes first depends on k. Repro: Tfidf([x y/0, apple pie/1]).rank("apple pie", 2) returns [x y, apple pie]; adding a third example puts apple pie first. A caller that truncates to fit a prompt keeps the wrong ones. F1 and F11 come from this same shortcut.
| def _fit(self) -> None: | ||
| # TfidfVectorizer rejects a corpus with nothing to put in its vocabulary. | ||
| sources = [ex.source for ex in self.examples] | ||
| if not any(self._tokenize(source) for source in sources): |
There was a problem hiding this comment.
FYI: F11. A corpus with no vocabulary gives [] from _rank, but the k >= n shortcut returns every example. Repro: ["!!!", "...", "??"] with rank("a", 2) returns [], while ["!!!", "..."] with rank("a", 2) returns both.
| if k <= 0: | ||
| return [] | ||
| num_examples = len(self.examples) | ||
| exclude_index = ( |
There was a problem hiding this comment.
FYI: F12. An integer exclude is never range-checked. Tfidf with exclude=-1 silently drops the last row on the ranked path, while the shortcut path ignores it. exclude=7 raises IndexError only when k < n, and exclude=True acts as index 1.
| def examples(self) -> List[Example]: | ||
| with self._lock: | ||
| if not isinstance(self._examples, List): | ||
| self._examples = list(self._examples) |
There was a problem hiding this comment.
FYI: F13. A caller's list is kept by reference, so appending to it after the first rank leaves the fitted matrix stale. Repro: rank(..., exclude=3) then raises IndexError: index 3 is out of bounds for axis 0 with size 3. Copy it, or document that the caller must not mutate it (AGENTS.md: be explicit about handle ownership). Unverified by me; reproduced by the code-review pass.
| return [ | ||
| self.examples[example_index] | ||
| for example_index in sorted( | ||
| (int(candidates[i]) for i in chosen), key=lambda i: (similarities[i], i), reverse=True |
There was a problem hiding this comment.
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.
| def _score_examples(self, query: str) -> np.ndarray: | ||
| if self._matrix is None or self._vectorizer is None: | ||
| return np.array([]) | ||
| index_of_query = next((i for i, ex in enumerate(self.examples) if ex.source == query), -1) |
There was a problem hiding this comment.
FYI: F5. This scans every source in Python on each query, which is O(n²) for leave-one-out over a whole Bible. transform([query]) gives the same vector, so dropping the lookup does not change the result. (Carried over from the earlier round.)
| self._vectorizer: Optional[TfidfVectorizer] = None | ||
| self._matrix: Optional[csr_matrix] = None | ||
|
|
||
| def _fit(self) -> None: |
There was a problem hiding this comment.
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.
| assert retriever.fit_started.wait(timeout=1) | ||
| second = executor.submit(retriever.rank, "b", 1) | ||
| try: | ||
| assert not retriever.second_fit_started.wait(timeout=0.2) |
There was a problem hiding this comment.
FYI: F16. This 0.2s timed wait still passes when the second thread has not reached the lock, so the test can pass with the lock removed. docs/review/machine-tests.md asks for no sleeps. Wait on a signal that the second thread is blocked instead.
|
|
||
| class ExampleRetrieverFactory: | ||
| @staticmethod | ||
| def create_example_retriever(method: ExampleRetrievalMethod, examples: Iterable[Example]) -> LazyExampleRetriever: |
There was a problem hiding this comment.
FYI: F8. ExampleRetrieverFactory has no test: the TFIDF, COVERAGE, and ValueError branches are never run. Also not covered: an exclude given as an Example through Tfidf, and Coverage with k >= n. (Carried over from the earlier round.)
API surface: this PR adds a new public package, Status of every finding (F1–F8 were raised in the earlier summary-only round; F1, F3, F5, and F8 are now posted as line comments):
Reviewed at 9125975 |
benjaminking
left a comment
benjaminking
left a comment
There was a problem hiding this comment.
@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.
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