fix: test kv flake with rephrase and chatcontext - #1403
Conversation
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
markstur
left a comment
markstur
left a comment
There was a problem hiding this comment.
A test named test_kv that doesn't validate that a kv is used in any way does bother me a little :)
See my inline comment. It might be a rant, but hopefully it's cool. Intended to share thoughts. I hope I'm not off base.
I think this is a solid improvement to merge as-is. Probably no action to take on my inline comment unless you feel like commenting what the test really does/doesn't do. It's kind of an odd test that just happened to run into privacy issues. Will be nice to have it pass and exercise more of the code it's supposed to exercise.
| model_options=dict(), | ||
| ) | ||
| result = await response.avalue() | ||
| assert "314" in result, f"Expected correct answer (314 main st) but found: {result}" |
There was a problem hiding this comment.
I don't think this test really tests anything useful other than call that function (which would otherwise lack tests). So personally I'd probably just comment out the assert -- or check for that perfectly valid safety refusal (or either, or any successful call, or whatever). The result doesn't really matter. This is just touching some lines of code and I think the safety message touches all the same code.
That said, I'll approve your approach if you think this is the best way to fix the issue at hand. Less flakey is more good. 👍
Also if the misleading test is really just for code coverage and not kv cache testing with validation, well you are significantly improving that with the ChatContext. It's touching more code and more KV-related code.
It could use a comment explaining why it says test_kv but doesn't actually test kv use. I know this isn't the scope of your issue but...
I cannot vouch for how accurate this is, but my bot explains it this way:
"This test validates that the KV-cache API endpoint works correctly. The 'spans' name refers to KV cache spanning multiple generation calls, but this test only validates a single call. True multi-call KV cache reuse testing is blocked by a Granite 4.1 compatibility issue."
ce2435b
…#1403) Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
2 participants
Pull Request
Issue
Fixes #398
Description
Rephrases prompt and uses chatcontext to fix test_kv flake.
Testing
Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.
NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.