Pass original request headers merged with enricher response headers to Pyfunc ensembler #217

krithika369 · 2022-07-20T04:30:29Z

This PR makes 2 user-facing changes:

engines/router/missionctl/handlers/http_handler.go - Make the Enricher's response headers available to the Ensembler in the request headers. The merging of the headers was introduced in Add steps to merge original request and enricher response headers #176, intended to be used for the routing step and we hadn't propagated it down to the Ensembler. However, allowing the Ensembler access to those values would help the Ensembler make decisions based on values computed by the Enricher.
Pass request headers to the Pyfunc Ensembler. Previously, request headers were not available to Pyfunc ensemblers. The changes are described in detail below.

Passing Ensembler Request Headers to the Pyfunc Ensembler

To support this, the optional **kwargs argument has been added to the EnsemblerBase.ensemble method, meant to take in contextual information. This will be used by the Pyfunc Ensembler Service to propagate request headers. The main changes are below.

engines/pyfunc-ensembler-service/pyfunc_ensembler_runner/handler.py - Nest request headers and body under a single dict, when calling the MlFlow Pyfunc model's predict.
sdk/turing/ensembler.py - Pick up the headers from the model input and pass them to the ensemble() method. The signature of the ensemble method now takes in **kwargs which should be included in the custom implementation to take advantage of the headers. However, since some users may be on the older Pyfunc ensemblers, a try-except has been added to issue the call without headers if there is an associated exception. This special handling can be removed in a few releases, when users have started to include **kwargs in their implementation.

Tests

The following scenarios have been tested to be working with the new Pyfunc service as of this PR:

Deploying images already built using older SDK versions
Building and deploying images from artifacts created by older SDK versions
Building and deploying images from artifacts created by the new SDK version as of this PR, with or without the **kwargs

krithika369 · 2022-07-21T06:17:23Z

engines/pyfunc-ensembler-service/pyfunc_ensembler_runner/ensembler_runner.py

-    def predict(self, inputs: Dict[str, Any]) -> List[Any]:
-        logging.info(f"Input request payload: {inputs}")
-        output = self._ensembler.predict(inputs)
+    def predict(self, body: Dict[str, Any], headers: Dict[str, str]) -> List[Any]:


This call's MLFlow Pyfunc model's predict which is eventually received here at runtime. The self.ensemble call from _ensemble_request finally calls the implementation provided by the user.

krithika369 · 2022-07-21T06:18:20Z

engines/pyfunc-ensembler-service/tests/test_ensembler_runner.py

@@ -20,19 +20,42 @@


 @pytest.mark.parametrize(
-    "inputs,expected", [
+    "ensembler_uri,inputs,headers,expected", [


Changes to test old and new ensemblers (created with and without **kwargs)

terryyylim

LGTM 🚀 , left some small comments!

terryyylim · 2022-07-21T06:48:17Z

sdk/turing/ensembler.py

@@ -1,4 +1,6 @@
 import abc
+import deprecation


I don't think this is being used right now, would using warnings library and raising a warning at except clause be better?

Thanks for catching, and the suggestion - removed the deprecation import and added a warning log.

terryyylim · 2022-07-21T06:57:54Z

engines/pyfunc-ensembler-service/pyfunc_ensembler_runner/ensembler_runner.py

-        logging.info(f"Input request payload: {inputs}")
-        output = self._ensembler.predict(inputs)
+    def predict(self, body: Dict[str, Any], headers: Dict[str, str]) -> List[Any]:
+        logging.info(f"Input request payload: {body}")


Is there any particular reason/benefit to log only the body but not headers (guessing it's because there might be auth details here)?

Right now, the default logger uses INFO level and it's not possible to change that. So I left out the header log, to not bloat the logs further. Will capture an item in the backlog to make logging configurable.

terryyylim · 2022-07-21T06:58:32Z

docs/how-to/create-a-router/configure-ensembler.md

@@ -26,7 +26,7 @@ It is not possible to select as the fallback response a route that has traffic r
 {% endhint %}

 ## Docker
-Turing will deploy specified image as a post-processor and will send the original request, responses from all routes, and the treatment configuration (if a Experiment Engine is selected, in Configure Experiment Engine), for ensembling. To configure a Docker ensembler, there are 3 sections to be filled.
+Turing will deploy the specified image as a post-processor and will send the in the request payload the original request, responses from all routes, and the treatment configuration (if an Experiment Engine is selected, in Configure Experiment Engine), for ensembling. The ensembler's request headers will contain the original request headers sent to Turing, merged with the enricher's response headers (if there are duplicates, the value in the enricher's response headers will take precedence). To configure a Docker ensembler, there are 3 sections to be filled.


"will send in the request payload to the original request" -> extra "the", missing "to"

Thanks! Rephrased for clarity.

terryyylim · 2022-07-21T07:02:55Z

engines/pyfunc-ensembler-service/tests/conftest.py

@@ -6,6 +6,22 @@

 class TestEnsembler(PyFunc):

+    def initialize(self, artifacts: dict):


Didn't notice this previously, but perhaps we should standardize the typings? I see different options - dict, Dict, Dict[with types].

Ideally yes. But there are also other inconsistencies like double vs single quote usage. We can later put in a linter to catch these everywhere and adjust styling. For now, I just changed this example of the test ensemblers to Dict to match theensemble method.

terryyylim · 2022-07-21T07:07:34Z

sdk/turing/ensembler.py

+        except TypeError as e:
+            if "got an unexpected keyword argument 'headers'" in str(e):
+                # This handles the legacy ensemblers
+                # TODO: Deprecate support for legacy ensemblers


Should perhaps specify what future SDK version would we be deprecating the legacy ensemblers.

I think deprecating this depends on users moving away as well. So we need to do it at some point, but I'm not sure when. We will also need to remove some of the default_route_id related stuff. Will also track this in the backlog.

deadlycoconuts

LGTM! 🚀 Thanks for cleaning up some of the stuff in the SDK files; @terryyylim also raised some interesting points which are worth thinking about so I just "+1"-ed some of them :)

docs/how-to/create-a-router/configure-ensembler.md

engines/pyfunc-ensembler-service/tests/test_ensembler_runner.py

krithika369 · 2022-07-21T10:03:55Z

Thanks for the quick review, both! I'll be merging this shortly if no further comments / questions.

Krithika Sundararajan added 4 commits July 20, 2022 08:19

Pass down Enricher response header to the ensembler request

476d653

Support request headers in the Pyfunc ensembler

8e4e81c

Update samples, tests and docs

4cd10a8

Update tests

3e35f39

krithika369 marked this pull request as draft July 20, 2022 04:30

Krithika Sundararajan added 4 commits July 20, 2022 12:47

Fix lint error

4157139

Update sample batch ensembler

0af31db

Use try-except to support legacy ensemblers

3421ce9

Minor changes

4029c9f

krithika369 marked this pull request as ready for review July 21, 2022 05:45

krithika369 requested a review from a team July 21, 2022 05:45

krithika369 commented Jul 21, 2022

View reviewed changes

terryyylim approved these changes Jul 21, 2022

View reviewed changes

deadlycoconuts approved these changes Jul 21, 2022

View reviewed changes

docs/how-to/create-a-router/configure-ensembler.md Show resolved Hide resolved

engines/pyfunc-ensembler-service/tests/test_ensembler_runner.py Show resolved Hide resolved

Address PR comments

3bb8287

krithika369 merged commit 3ae8f23 into caraml-dev:main Jul 21, 2022

krithika369 deleted the pyfunc_ensembler_headers branch July 21, 2022 10:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass original request headers merged with enricher response headers to Pyfunc ensembler #217

Pass original request headers merged with enricher response headers to Pyfunc ensembler #217

krithika369 commented Jul 20, 2022 •

edited

Loading

krithika369 Jul 21, 2022

krithika369 Jul 21, 2022

terryyylim left a comment

terryyylim Jul 21, 2022

krithika369 Jul 21, 2022

terryyylim Jul 21, 2022

krithika369 Jul 21, 2022

terryyylim Jul 21, 2022

krithika369 Jul 21, 2022

terryyylim Jul 21, 2022

krithika369 Jul 21, 2022 •

edited

Loading

terryyylim Jul 21, 2022

krithika369 Jul 21, 2022

deadlycoconuts left a comment

krithika369 commented Jul 21, 2022

		@@ -6,6 +6,22 @@

		class TestEnsembler(PyFunc):

		def initialize(self, artifacts: dict):

Pass original request headers merged with enricher response headers to Pyfunc ensembler #217

Pass original request headers merged with enricher response headers to Pyfunc ensembler #217

Conversation

krithika369 commented Jul 20, 2022 • edited Loading

Passing Ensembler Request Headers to the Pyfunc Ensembler

Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terryyylim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krithika369 Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deadlycoconuts left a comment

Choose a reason for hiding this comment

krithika369 commented Jul 21, 2022

krithika369 commented Jul 20, 2022 •

edited

Loading

krithika369 Jul 21, 2022 •

edited

Loading