CV2-5011 refactors for making alegre dual purpose on text encoding #103
Conversation
It does not - the only thing that's happening here is basically updating vectorizers to return what is now the current specification for how responses from fingerprinters should look - this one got out of sync since we hadn't been using it for anything. Mostly just tagging you here for visibility so you can be aware of the magnitude of changes across each repo, don't worry too much about the specifics in this repo! |
lib/model/generic_transformer.py
Outdated
There was a problem hiding this comment.
I realize this file has existed for a while, but is there a reason we override the respond() function instead of process()?
this would make this model unique and different from other Presto endpoints, and disables cache and error handling bc the standard get_response() function won't be called from model.py.
But I may be missing something?
There was a problem hiding this comment.
Yes - the reason we override is because we want to, from the get-go, natively be able to process jobs in batch on transformers instead of just walking through items one-by-one like we do with the others. I'll look into a refactor to keep our caching and error handling working for text though - good catch
There was a problem hiding this comment.
the only thing that's happening here is basically updating vectorizers to return what is now the current specification for how responses from fingerprinters should look - this one got out of sync since we hadn't been using it for anything.
I'm not very clear how the output changed, but it looks like it will handle caching better? Is the current specification somewhere I can check if it matches? I'm just assuming it works because you said you tested it locally
That's basically it @skyemeedan - the function calling the fingerprinting function expects a slightly different response. Once we have the fixes from @ashkankzme on the PR he's working on it should be a bit easier to highlight conceptual float like this case. |
There was a problem hiding this comment.
This looks fine for now. Thanks!
When we work on the bulk endpoints for Alegre, I think we're going to want one request to Presto to have a list of multiple text items to be vectorized. All of the vectors for those items should then be returned in one callback. I think some of the changes here may complicate that, but we can address it when we get to the bulk endpoints on Alegre.
test/lib/queue/test_queue.py
Outdated
| self.model = GenericTransformerModel(None) | ||
| self.model.model_name = "generic" | ||
| self.model = AudioModel() | ||
| self.model.model_name = "audio" |
There was a problem hiding this comment.
can we make sure model_name follows the same format as everywhere else, i.e. audio__Model? these names would break the tests in the new refactor
Description
Minor tweak identified when running text encodings.
Reference: CV2-5011
How has this been tested?
Tested locally and confirmed to work
Are there any external dependencies?
Changes no dependencies
Have you considered secure coding practices when writing this code?
Does not alter security posture.