Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[hma][api] Add option to return banks in matching API #1653

Closed
wants to merge 2 commits into from

Conversation

andrew-dillon
Copy link

Summary

Include the list of matching banks in the /m/raw_lookup&include_distance=true response.

Test Plan

Updated the src/OpenMediaMatch/tests/test_e2e_workflow.py test.

@Dcallies Dcallies changed the title Add banks [hma][api] Add option to return banks in matching API Oct 6, 2024
Copy link
Contributor

@Dcallies Dcallies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @andrew-dillon, thanks for contributing to HMA! I think having the ability to lookup content by banks is valuable, though if you explain how you plan on using this in production we can make sure you get the very best version of this functionality.

As written, you will cause a performance regression in the lookup API, because resolving the banks requires another roundtrip to the database. Additionally, the way you are calling it in this PR will duplicate the index lookup! We want to try and maintain a very fast query option for the index since it's essentially the key service of HMA!

Based on my guess of how you might want to use this functionality, here's what I recommend:

  1. Reply with the higher level problem you want to solve
  2. Do not query the index twice, and keep the _SignalIndexInMemoryCache as lightweight as possible - it's used by other functions!
  3. At the API where you wish to receive the banks Only resolve the banks if the API receives an optional parameter, similar to how this is handled on L117.

@@ -39,7 +39,7 @@
class MatchWithDistance(t.TypedDict):
content_id: int
distance: str

banks: list[str]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking: A content_id can only ever be in one bank, so I think this should be similar.

Comment on lines +154 to 155
banks = lookup(signal, signal_type_name)
results = query_index(signal, signal_type_name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking: This is essentially causing us to double query to the index, since lookup will query the index, and query_index also queries the index.

Instead, what you want to be doing is taking the returned content_ids and doing a lookup on which (singular) bank they are in, which you can essentially see how to do in the body of lookup.

@andrew-dillon
Copy link
Author

Hey @Dcallies! Thanks for looking over this. We don't need these changes right now so I'll go ahead and close this.

@Dcallies
Copy link
Contributor

No worries! We do need to do some work on the match APIs, @juanmrad and I have been talking about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants