[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 94.14062% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.29%. Comparing base (abe3718) to head (f6d230a).
⚠️ Report is 5 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #417      +/-   ##
==========================================
+ Coverage   92.27%   92.29%   +0.01%     
==========================================
  Files         398      402       +4     
  Lines       25461    25715     +254     
==========================================
+ Hits        23494    23733     +239     
- Misses       1967     1982      +15     

☔ 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).

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

[benjaminking]

It looks like there are a few different ways that OpenRouter handles exceptional conditions. For quite a few, it just represents them with HTTP codes. In SILNLP, we've been working through LiteLLM, which handles some of these, so the exact list of what must be handled is different. The most important part is figuring out which failures should lead to retrying, which ones should lead to skipping a query, and which ones should cause the job to fail. Claude was able to give me a pretty good breakdown of what all the various HTTP codes mean in the context of OpenRouter, which cases the OpenRouter servers and API handle, and which ones would need to be handled in our code.

For timeouts and retries, it's possible that OpenRouter's behavior might be sufficient, but I'd just take a look at whether there are any cases that we might want to handle differently in our context.

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

[claude]

Minor: F12. A per-call temperature or max_completion_tokens is silently dropped: the fixed fields are applied after kwargs, so they win. With max_tokens=100 in the constructor, complete(msgs, temperature=0.7, max_completion_tokens=5) sent temperature=0.0 and max_completion_tokens=100, with no error. Either let kwargs override these, or raise TypeError on reserved keys, as the model test implies is intended for model.

[claude]

F12 still applies at 1f1a587: temperature is fixed, because it is no longer set after kwargs, but max_completion_tokens still is (line 56). With max_tokens=100, complete([], temperature=0.7, max_completion_tokens=5) now sends temperature=0.7 and max_completion_tokens=100. Apply max_tokens before request.update(kwargs), or reject the key.

[Enkidu93]

OK, I've taken a closer look at what openrouter retries on and when it throws, and it seems like its retry logic/defaults are very reasonable. If it still hits a 4--/5-- code after the retry policy, it throws that code. The only code I can see that we may want to retry on additionally would be 429s, but we could also just update the retry config passed to the client rather than having nested retries. In any case, I think any additional logic should probably go in the calling code. Just to get ahead of this discussion though, you mentioned status codes or errors where we may just want to skip a verse? Any specific cases you had in mind? I don't love the idea (especially for the LLM drafting) of returning a draft with verses missing. One thing that came to mind was a content filter block: What's your experience with those? Worth retrying? Or just give up? If those need to be handled separately, add something now to distinguish those failures.

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

---

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

Previously, Enkidu93 (Eli C. Lowry) wrote…

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.

I just added kwargs since I think we'll probably want that kind of flexibility in silnlp.

[benjaminking]

Thanks for taking a look at that. The other case I was thinking of where we might need to skip is a content filtering block. I think you're right though that it is probably worth retrying those lines -- I know content blocks are not always deterministic.

I do think we should probably implement logic for HTTP 429. That one has come up a fair bit in practice. I'd be fine with updating the retry config.

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

[mshannon-sil]

My experience with the content blocking when testing postediting on certain providers, is that it's fairly deterministic. Retrying verses has not seemed to help, and I've had to instead rerun it and omit certain verses from the examples to avoid triggering it. I don't think that's the greatest solution (what happens when the test verse itself is blocked), but I don't think it's worth waiting multiple minutes on retries for blocked verses if nothing will change.

I was going to add something for this in llm postediting, but it probably makes more sense to handle it here since it potentially affects all remote llm calls to certain providers.

@mshannon-sil reviewed 1 file, made 1 comment, and resolved 1 discussion.
Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion (waiting on ddaspit and Enkidu93).

[Enkidu93]

OK, that's helpful! I will see what I can do. Do you think maybe the best thing with content-blocking is just to throw a unique error and let the client do what it may? I could see the post-processing, for example, just not caring since it's kinda icing on top of an existing draft whereas the remote drafting might be more dogged.

@Enkidu93 made 1 comment and resolved 1 discussion.
Reviewable status: 7 of 8 files reviewed, all discussions resolved (waiting on ddaspit and mshannon-sil).

[benjaminking]

I think that's probably what we should do. Because, as you mentioned, different clients might want to do different things when this happens. A client that batches (processing multiple verses in a single query) will probably want to try to split its request into multiple queries and re-try.

One part that could make this tricky is that we may not be guaranteed to get a 429. In previous experiments, I've sometimes gotten a normal response that says "Sorry, I can't help with that," or something similar. Ideally, we'd like to treat that case the same as a 429, but that's probably out-of-scope for the work we're doing right now.

@benjaminking made 1 comment.
Reviewable status: 7 of 8 files reviewed, all discussions resolved (waiting on ddaspit and mshannon-sil).

[Enkidu93]

Alright, I've added a content block error. If you guys would prefer a different naming or anything, let me know.

@Enkidu93 made 1 comment.
Reviewable status: 5 of 8 files reviewed, all discussions resolved (waiting on ddaspit and mshannon-sil).

[claude]

Major: F15. Every CI build is red because isort rejects this import order. ContentBlockError must come before OpenRouterLlmCompletionClient, here and in tests/translation/llm/test_openrouter_llm_completion_client.py:7. Job 131285580193 (macOS 3.13) fails at uv run isort . --check-only with Imports are incorrectly sorted for both files, and fail-fast cancels the other 14 builds. Running isort on the two files fixes it.

[claude]

FYI: F16. ContentBlockError puts the whole prompt in its message, so every log line or traceback that shows the error also prints all of the source text. With a 2,000-character system prompt, str(e) was 2,150 characters long and included the user text. A short summary, such as the message count, or the messages kept as an attribute on the exception, would keep logs small.

[claude]

FYI: F17. This match= does not check the message. pytest reads match as a regex, and str(messages) starts with [, so the pattern is one character class that matches almost any text. pytest.raises(ContentBlockError, match=str(messages)) passed against ContentBlockError("unrelated message without context"). Use re.escape(str(messages)).

[claude]

Verdict: request changes. The new ContentBlockError works, but the commit breaks CI.

The one thing that matters most is F15 at machine/translation/llm/__init__.py:2. isort rejects the new import order there and in the test file, so every build fails or is canceled.

Counts: 1 Critical (F15, new), 1 Important (F12, open), 2 Low (F16, F17, new).

Ran: gh api .../compare/558ad40...221be87 returned ahead. git diff 558ad40 221be87 adds ContentBlockError, which is raised on finish_reason == "content_filter", plus one test. gh pr checks 417: macOS 3.13 fails at uv run isort . --check-only for both files, the other 14 builds are canceled, and claude-review and Reviewable are pending. .venv/bin/python -m pytest tests/translation/llm/test_openrouter_llm_completion_client.py gave 15 passed. With a mocked SDK, a 2,000-character prompt produced a 2,150-character error message (F16). pytest.raises(..., match=str(messages)) also passed on an unrelated message (F17). The max_completion_tokens override is unchanged at line 64 (F12).

Not verified: live OpenRouter behavior for :free slugs (F8), for truncated replies (F2), and whether OpenRouter reports blocks as content_filter. No network calls were made.

Surface changed: machine.translation.llm now exports ContentBlockError, and complete() raises it on a content-filtered reply instead of returning a Completion. Not released yet. No parity contract with sillsdev/machine applies.

Findings: F1 addressed, F2 addressed (no-choices part; finish_reason part still accepted, unverified), F3 addressed, F4 addressed, F5 addressed, F6 addressed, F7 addressed, F8 accepted, unverified, F9 addressed, F10 addressed, F11 withdrawn, F12 open, F13 addressed, F14 addressed, F15 new, F16 new, F17 new.

Reviewed at 221be87

[mshannon-sil]

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