Logging post inference hook implementation #428
Conversation
|
This pull request has been linked to Shortcut Story #835062: Implement post_inference_hook to write to firehose. |
model-engine/model_engine_server/inference/post_inference_hooks.py
Outdated
Show resolved
Hide resolved
model-engine/model_engine_server/inference/post_inference_hooks.py
Outdated
Show resolved
Hide resolved
model-engine/model_engine_server/inference/post_inference_hooks.py
Outdated
Show resolved
Hide resolved
model-engine/model_engine_server/inference/post_inference_hooks.py
Outdated
Show resolved
Hide resolved
model-engine/model_engine_server/inference/post_inference_hooks.py
Outdated
Show resolved
Hide resolved
model-engine/model_engine_server/inference/domain/gateways/streaming_storage_gateway.py
Outdated
Show resolved
Hide resolved
| endpoint_type=endpoint_config.endpoint_type, | ||
| bundle_id=endpoint_config.bundle_id, | ||
| labels=endpoint_config.labels, | ||
| streaming_storage_gateway=FirehoseStreamingStorageGateway(), |
There was a problem hiding this comment.
should we actually call get_external_interfaces() before and use the monitoring_metrics_gateway and streaming_storage_gateway from there?
There was a problem hiding this comment.
yeah that could work
There was a problem hiding this comment.
seems like using external interfaces doesn't work, I can't import from api in inference
There was a problem hiding this comment.
wait is this where the async forwarder is implemented? if not, I think this code might be dead code (i.e. it was used back when async task receive/push-result would run in the same container as the user code
There was a problem hiding this comment.
if the code is dead code, then I think the import would work because there was at some point some amount of copying only part of the code to the user inference container because of security reasons
There was a problem hiding this comment.
yeah this is dead code so the import works but I'm referring to everywhere else (like in forwarding.py) it wouldn't work. also, if this is dead code anyways, then there's not really a point in importing from external interfaces?
There was a problem hiding this comment.
IMO we could probably just delete the dead code (as long as we're sure it's dead)
re forwarding.py I think it should be possible to make ExternalInterfaces importable; I guess since we have stuff in inference/{domain|infra}/gateways the current layout of things is a bit different; I still think that in the ideal state we'd be able to use the same plugins mechanism to control which interfaces we're using
There was a problem hiding this comment.
hm how do we make it possible to be importable in forwarding.py? yeah ideally it does make sense to control which interfaces we're using.
There was a problem hiding this comment.
For the sake of rolling this feature out soon, I think for now I'll leave the 2 gateways like this and can always follow-up with deleting dead code / using ExternalInterfaces
model-engine/model_engine_server/inference/infra/gateways/firehose_streaming_storage_gateway.py
Outdated
Show resolved
Hide resolved
model-engine/model_engine_server/inference/post_inference_hooks.py
Outdated
Show resolved
Hide resolved
model-engine/model_engine_server/inference/infra/gateways/firehose_streaming_storage_gateway.py
Outdated
Show resolved
Hide resolved
Pull Request Summary
Logging post inference hook to firehose stream.
Test Plan and Usage Guide
Runs successfully locally. Load test of 1000 requests success (finished in < 1min and all show up in table).