[Enkidu93]

Resolves #398.

---

This change is 

[Enkidu93]

@Enkidu93 made 10 comments and resolved 9 discussions.
Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on benjaminking, ddaspit, and mshannon-sil).

[codecov-commenter]

Codecov Report

❌ Patch coverage is 93.78238% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.28%. Comparing base (abe3718) to head (375745b).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...ranslation/llm/openrouter_llm_completion_client.py	89.85%	7 Missing ⚠️
machine/translation/llm/llm_completion_client.py	84.00%	4 Missing ⚠️
...ation/llm/test_openrouter_llm_completion_client.py	98.93%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #417      +/-   ##
==========================================
+ Coverage   92.27%   92.28%   +0.01%     
==========================================
  Files         398      402       +4     
  Lines       25461    25652     +191     
==========================================
+ Hits        23494    23673     +179     
- Misses       1967     1979      +12     

☔ 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]

@Enkidu93 made 2 comments.
Reviewable status: 0 of 8 files reviewed, 1 unresolved discussion (waiting on benjaminking, ddaspit, and mshannon-sil).

[claude]

Verdict: approve. Every finding is now settled. The only change since 934d640 restores .vscode/settings.json to match main, which addresses F10.

The one point that still matters is F2 at machine/translation/llm/openrouter_llm_completion_client.py:56, which the author has knowingly kept until a human use-case review: a response with no choices comes back as an ordinary empty Completion, and finish_reason is not exposed.

Counts: 0 Critical, 0 Important, 0 Low new. F2 (Important) is accepted.

Ran: gh api .../compare/934d640...375745b returned ahead, and git diff 934d640 375745b touches only .vscode/settings.json. git diff 6be62fd 375745b -- .vscode/ is empty. I did not run the code review again because no Python changed. gh pr checks 417: macOS 3.14 build passes; the other 14 builds, claude-review, and Reviewable are pending, so lint, type, and test status is not known yet.

Not verified: live OpenRouter behavior for :free slugs (F8) and for truncated replies (F2). No network calls were made.

Surface changed: new public machine.translation.llm subpackage (OpenRouterLlmCompletionClient, Completion, LlmCompletionClient, TokenLogprob), a new llm optional dependency, and the exported machine.translation.TranslationEngine is now an ABC. The author says this is intentional, so it is worth a release note. No parity contract with sillsdev/machine applies.

Findings: F1 addressed, F2 accepted (truncation part unverified), F3 addressed, F4 addressed, F5 addressed, F6 addressed, F7 addressed, F8 accepted, unverified, F9 addressed, F10 addressed, F11 withdrawn.

Reviewed at 375745b

[mshannon-sil]

@mshannon-sil reviewed 8 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on benjaminking, ddaspit, and Enkidu93).

---

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

        self,
        model: str,
        temperature: float = 0.0,

Will there be a way to pass in additional parameters or do we need to add them here? Some things I'd want to be able to specify are seed, provider, and thinking level.

[benjaminking]

A few thoughts about exceptional behavior:

1. It looks like there are quite a few different types of errors that could occur when you call send(). Some of them may be worth handling here, other probably at the level of the callers.

2. Are you thinking that concurrency and retries will exist in a layer above this? Those are important, but fine to leave out for now, if they're going to be implemented in a wrapper.

3. Timeouts are another thing we probably need to support. That probably is best handled in this class.

@benjaminking reviewed all commit messages and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on ddaspit and Enkidu93).

[Enkidu93]

Thank you, Ben!

1. What do you have in mind here? HTTP response status codes? Malformed response content of some kind? I've only had experience with the API in testing and in reading the docs, so I'm not perhaps familiar enough with the kinds of things that could go wrong. I know you've recently done some work in silnlp to handle some of these sorts of things more gracefully, so maybe you have some particular ideas?
2. Concurrency I expect to be handled at the layer above this. As for retries, the openrouter python client seems to have pretty sensible default retry behavior unless I'm misreading things. If you think we need a custom behavior, I can override that obviously.
3. The provided client also claims to gracefully handle timeouts. Is there a particular situation you had in mind for us to address expressly here?

Let me know if I'm misunderstanding something.

@Enkidu93 made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on ddaspit).

[Enkidu93]

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

---

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

Previously, mshannon-sil wrote…

Will there be a way to pass in additional parameters or do we need to add them here? Some things I'd want to be able to specify are seed, provider, and thinking level.

Good call. Yeah, I wasn't sure what all you need. I'll separate these parameters out into a separate configuration option. If you think of others, let me know. Alternatively, we could allow passing arbitrary options through kwargs if you expect to need that flexibility.