add similarity to raw_lookup endpoint #1639
Conversation
| """ | ||
| signal = require_request_param("signal") | ||
| signal_type_name = require_request_param("signal_type") | ||
| return lookup_signal(signal, signal_type_name) | ||
|
|
||
|
|
||
| def lookup_signal(signal: str, signal_type_name: str) -> dict[str, list[int]]: | ||
| def lookup_signal(signal: str, signal_type_name: str) -> dict[str, dict[str, str]]: |
There was a problem hiding this comment.
I like this response shape, but just noting that it is technically a breaking change for clients of this API
There was a problem hiding this comment.
agreed, probably the right thing to do is version the api change
|
Related to #1573 |
prenner
force-pushed
the
prenner/add-similarity-score
branch
from
93e50ea
to
e9e823d
Compare
There was a problem hiding this comment.
Nice job! This ended up being simpler than I had thought it would be.
Tests are not expected to 503!
I believe the only path that can lead to 503 is if the index is not available when you try and query it. Are you saying they are 503-ing in main or just when you make the changes?
| if entry is not None and is_in_pytest(): | ||
| entry.reload_if_needed(get_storage()) |
There was a problem hiding this comment.
blocking q: Hmm, I'm very suspicious of any branching in the code that specifically changes behavior in tests.
Is there another solution that you can use in the test itself that can achieve the same results? (e.g. disabling background loading in the fixture for the test? Manually calling a load_all_indices() function?)
| def is_in_pytest(): | ||
| return "pytest" in sys.modules |
There was a problem hiding this comment.
Danger! I don't think this works consistently, and someone could accidentally include a test library in the build that silently changes the behavior of HMA. This is why I am generally suspicious of this kind of branching logic!
There was a problem hiding this comment.
ha agreed, glad we found the root cause!
| """ | ||
| signal = require_request_param("signal") | ||
| signal_type_name = require_request_param("signal_type") | ||
| return lookup_signal(signal, signal_type_name) | ||
| include_distance = bool(request.args.get("include_distance", False)) == True |
There was a problem hiding this comment.
This works for me as a solution, though I think this bool parsing is different than what we do for other flags.
blocking: Can you include it in the list of inputs in the docstring?
|
|
||
| return lookup_signal_func(signal, signal_type_name) |
There was a problem hiding this comment.
nit: Both of these functions return the same shape, it might be slightly easier to have this return the list and do
return {
"matches": lookup_signal_func()
}
Summary
Test Plan
to run api tests: