-
Notifications
You must be signed in to change notification settings - Fork 0
[CV2-5117] batch formats #111
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,178 @@ | ||
| # Expected incoming request and outgoing response datastructures for Presto models | ||
|
|
||
| Presto models should use these structures to ensure that tools that consume the data can access in a consistant way across models | ||
|
|
||
| Details of specific input formats are usually in the schema.py fiels | ||
|
|
||
| ## Incoming requests to presto | ||
|
|
||
| General design | ||
|
|
||
| * Models that don't handle requests in parallel need to implement batch format (but can give errors if they recieve more than 1 item) | ||
| * "top level" elements are processed and controlled by the Presto system | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be beneficial to specify exactly what we mean by 'top level' here, i.e. is it specific fields (we should name if so) or are there more generic rules for defining 'top level' elements?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, it is specific fields (which we should name) and the general concept that the outermost set of keys talk to Presto, and the next level down talk to the specific presto model service. |
||
| * Elements inside parameters, and inside individual input items are passed through to the model | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a reason to not pass other parameters into the models? it's good to have the flexibility of using other parameters upon need if doing so is not disruptive
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, this is confusing. By "top level" I mean the outer-most set of dictionary keys in the payload. I think everything is passed through to the model? But Presto is responsible for enforcing and checking keys at the outermost level, but shouldn't really enforce or check inside parameters or items lists (other than that ids exist). Anything that isn't in the presto schema isn't promised to be passed through (we could always add to the schema if it becomes too constraining). I think greater flexibility is often faster in the short term but then ends up with everything having to do lots of conditional checking about elements that may or may not exist vs being able to check and enforce schemas and fail back to caller before enqueuing malformed messages to models |
||
| * All of the content in a batch (a single html request) must go to the same model | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. future versions of classycat with local classification could receive an input batch, some of which to be processed locally (the items with previous similar references) and some processed with an LLM (more novel items). I would say this is a desired case for one batch being processed by two different models and the current architecture allows for it. The outputs would ofc still be as if all processed by the same model. It would be good to have clarity on what we exactly mean here by 'all items in a batch go to the same model'?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess I mean more precisely, "from the perspective of the calling service, all the of the items in a batch must be dispatched to the same Presto model service". Classycat can of course internally decide to process via different LLMs "under the hood" but don't mix payloads intended for classycat with payloads intended for paraphrase multilingual in the same call. |
||
| * Any parameter settings to the model must apply to all items in batch | ||
| * Request itself needs a unique id, (and this is different than the id of the content) | ||
| * There is single callback URL for entire batch | ||
| * Some items need additional input parameters per item (i.e. “language” for YAKE), Presto just passes thes through | ||
| * All input_items must include an `id` element that can be used as a reference from the output items to match up paramters | ||
|
|
||
| Example PROPOSED input object structure (classycat) | ||
| ``` | ||
| { | ||
| request_id:122, | ||
| model_name:"classycat__Model", # model to be used | ||
| model_parameters: { | ||
| "event_type": "classify", | ||
| "schema_id": "4a026b82-4a16-440d-aed7-bec07af12205", | ||
| }, | ||
| input_items:[ | ||
| { | ||
| id:"11", | ||
| text:"this is some text to classify", | ||
| workspace_id:"timpani_meedan", | ||
| target_state:"classified", | ||
| }, | ||
| { | ||
| id:"12", | ||
| text:"Este é um texto para classificar", | ||
| workspace_id:"timpani_meedan", | ||
| target_state:"classified", | ||
| }, | ||
| ], | ||
| callback_url: "http://conductor/batch_reply/", | ||
| } | ||
|
|
||
| ``` | ||
| Example PROPOSED input object structure (YAKE) | ||
| { | ||
| request_id:422, | ||
| model_name:"yake_keywords__Model", # model to be used | ||
| model_parameters: { | ||
| "max_ngram_size":3, | ||
| "deduplication_threshold":0.25 | ||
| }, | ||
| input_items:[ | ||
| { | ||
| id:"11", | ||
| text:"this is some text to classify", | ||
| workspace_id:"timpani_meedan", | ||
| target_state:"keyworded", | ||
| language:"en", | ||
|
|
||
| }, | ||
| ], | ||
| callback_url: "http://conductor/batch_reply/", | ||
| } | ||
|
|
||
|
|
||
| Example PROPOSED input object structure (paraphrase-multilingual) | ||
|
|
||
|
|
||
| Example 'curl' command | ||
|
|
||
|
|
||
| ## Outgoing requests from presto | ||
|
|
||
| * The outgoing reponse request includes the payloads and parameters from the incoming request | ||
| * items in the response must to include their id, so that the caller can match individual-level properties from incoming request. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo: extra 'to' |
||
| * status code is present at top level so it can be checked to know how to parse the rest of the payload (error etc) | ||
|
|
||
| Example PROPOSED output structure | ||
| ``` | ||
| { | ||
| request_id:122, | ||
| model_name:"classycat__Model", # model to be used | ||
| model_parameters: { | ||
| "event_type": "classify", | ||
| "schema_id": "4a026b82-4a16-440d-aed7-bec07af12205", | ||
| }, | ||
| input_items:[ | ||
| { | ||
| id:"11", | ||
| text:"this is some text to classify", | ||
| workspace_id:"timpani_meedan", | ||
| target_state:"classified", | ||
| language:"en" | ||
|
|
||
| }, | ||
| { | ||
| id:"12", | ||
| text:"Este é um texto para classificar", | ||
| workspace_id:"timpani_meedan", | ||
| target_state:"classified", | ||
| language:"pt" | ||
| }, | ||
| ], | ||
| callback_url: "http://conductor/batch_reply/", | ||
|
|
||
| # elements below here are added in response | ||
|
|
||
| "retry_count": 0 | ||
| "status_code": 200 | ||
| "status_message":"success" | ||
| "results": [ | ||
| { | ||
| "id": "11", # this id can be used to look up params from input | ||
| "text": "this is some text to classify", # text is redundant with input, but helpful for model-level debugging | ||
| "labels": [ | ||
| "Testing", | ||
| "Classification" | ||
| ] | ||
| }, | ||
| { | ||
| "id": "12", | ||
| "text": "Este é um texto para classificar", | ||
| "labels": [ | ||
| "Testing", | ||
| "Classification" | ||
| ] | ||
| } | ||
| ] | ||
| }, | ||
|
|
||
| ``` | ||
|
|
||
| Example PROPOSED output object structure (YAKE) | ||
| { | ||
| request_id:422, | ||
| model_name:"yake_keywords__Model", # model to be used | ||
| model_parameters: { | ||
| "max_ngram_size":3, | ||
| "deduplication_threshold":0.25 | ||
| }, | ||
| input_items:[ | ||
| { | ||
| id:"11", | ||
| text:"this is some text to classify", | ||
| workspace_id:"timpani_meedan", | ||
| target_state:"keyworded", | ||
| language:"en", | ||
|
|
||
| }, | ||
| ], | ||
| callback_url: "http://conductor/batch_reply/", | ||
| "retry_count": 0 | ||
| "status_code": 200 | ||
| "status_message":"success" | ||
| "results": [ | ||
| { | ||
| "id": "11", # this id can be used to look up params from input | ||
| "text": "this is some text to classify", | ||
| "keywords": [ | ||
| ["lookup",0.0020211625251083634], | ||
| ["params",0.0020211625251083634] | ||
| ] | ||
| }, | ||
|
|
||
| ] | ||
| } | ||
|
|
||
| # Error response structures | ||
|
|
||
| "status_code": 403 | ||
| "status_message":"unable to access model" | ||
|
|
||
| ## Not covered | ||
| per item errors? | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one thing I tried to start in the previous refactor of
parse_message()was to have these model specific types defined in the model / in a separate file thanschemas.py(e.g.classycat_response.py). going forward we don't want to keep updatingschemas.pyfor new models/edits to existing models, and instead update the files specific to the model under construction/review. I have already filed a ticket for taking out whatever 'response classes' defined in theschemas.pyfile, see this jira ticketso I would edit this line to include the example of
classycat_response.pyand emphasize on defining these models separately going forward.