Thanks for the suggestion. The suggested approach looks good to me and wouldn't break the cross-language wrapper as far as RunInference wrapper for Go SDK (in process) is concerned.
On Fri, Sep 30, 2022 at 10:48 AM Andy Ye via dev <dev@beam.apache.org> wrote: > Thanks Anand! I think following this framework will help developers create > ModelHandlers with fewer pitfalls, especially for specialized frameworks > like XGBoost that support a unique set of params for predict(). > > On Thu, Sep 29, 2022 at 11:16 AM Danny McCormick via dev < > dev@beam.apache.org> wrote: > >> Thanks for the writeup Anand, I'm +1 on your recommendation in the doc: >> when we want to make parameterized changes that will propagate to the >> ModelHandler, I think we should modify the ModelHandler constructor rather >> than trying to propagate extra arguments through the run_inference function. >> >> Thanks, >> Danny >> >> On Thu, Sep 29, 2022 at 10:41 AM Anand Inguva via dev < >> dev@beam.apache.org> wrote: >> >>> Hi, >>> >>> Recently I encountered a breaking change when adding an additional >>> parameter to the RunInference transform[1] and ModelHandler[2] in >>> pull/23266 <https://github.com/apache/beam/pull/23266>. >>> >>> I explained what happened in a doc[3] and provided a few suggestions on >>> how to avoid this kind of behavior. Please take a look and let me know what >>> you think. >>> >>> Thanks, >>> Anand >>> >>> [1] >>> https://github.com/apache/beam/blob/17453e71a81ba774ab451ad141fc8c21ea8770c9/sdks/python/apache_beam/ml/inference/base.py#L291 >>> [2] >>> https://github.com/apache/beam/blob/17453e71a81ba774ab451ad141fc8c21ea8770c9/sdks/python/apache_beam/ml/inference/base.py#L109 >>> [3] >>> https://docs.google.com/document/d/1vJgGArgNTCkKo9BDxhf5jJtzr6yLTo1CRMw5B9ljT9o/edit?resourcekey=0-5TTi0_4oFPl0DM_g_04JBA# >>> >>
