Pass missing infer parameters during conversion #3368

sivanantha321 · 2024-01-16T12:06:06Z

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #3365

Type of changes
Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Feature/Issue validation/testing:

Please describe the tests that you ran to verify your changes and relevant result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.

Test A
Test B
Logs

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Checklist:

Have you added unit/e2e tests that prove your fix is effective or that this feature works?
Has code been commented, particularly in hard-to-understand areas?
Have you made corresponding changes to the documentation?

Release note:

Pass missing infer parameters during conversion

ajstewart · 2024-01-18T19:22:32Z

I saw this PR linked to my issue and had a look out of interest as I wondered about the to_grpc method. In my own work around I had to make sure that the to_grpc was setting the parameters as kserve.protocol.grpc.grpc_predict_v2_pb2.InferParameter objects. This is in the transformer. Then in the predictor I had to do some conversion back to either string, int, float or bool with prior knowledge as to what they should be (I couldn't see a helper on that object to transform back to the intended format).

This is all new to me and I'm not very familiar with kserve so I might be talking nonsense but I couldn't see an elegant way to deal with that.

I must admit I'm using them to place extra information into the request to change some predictor behaviour and some custom component settings, so I could be abusing the functionality.

sivanantha321 · 2024-01-19T09:04:05Z

I saw this PR linked to my issue and had a look out of interest as I wondered about the to_grpc method. In my own work around I had to make sure that the to_grpc was setting the parameters as kserve.protocol.grpc.grpc_predict_v2_pb2.InferParameter objects. This is in the transformer. Then in the predictor I had to do some conversion back to either string, int, float or bool with prior knowledge as to what they should be (I couldn't see a helper on that object to transform back to the intended format).

This is all new to me and I'm not very familiar with kserve so I might be talking nonsense but I couldn't see an elegant way to deal with that.

I must admit I'm using them to place extra information into the request to change some predictor behaviour and some custom component settings, so I could be abusing the functionality.

Can you try with these changes and give some feedback ?

python/kserve/kserve/protocol/infer_type.py

yuzisun · 2024-01-22T13:58:46Z

The change looks good, discussed with @Siva to add a few tests

sivanantha321 · 2024-01-24T08:42:32Z

The change looks good, discussed with @Siva to add a few tests

Tests added.

sivanantha321 · 2024-01-24T08:43:59Z

python/kserve/test/test_server.py

@@ -240,14 +243,14 @@ def test_list_models_v1(self, http_server_client):

    def test_predict_v1(self, http_server_client):
        resp = http_server_client.post('/v1/models/TestModel:predict',
-                                       data=b'{"instances":[[1,2]]}')
+                                       content=b'{"instances":[[1,2]]}')


To fix the warning
DeprecationWarning: Use 'content=<...>' to upload raw bytes/text content.

ajstewart · 2024-01-24T13:41:05Z

Can you try with these changes and give some feedback ?

A gave your branch a go (3a773e7) but I'm hitting an assignment error pasted below. I think I remember hitting the same problem in my workaround and I ended not trying to reassign on the current parameters but instead recreating them.

Traceback (most recent call last):
  File "/usr/local/lib/python3.10/site-packages/uvicorn/protocols/http/httptools_impl.py", line 419, in run_asgi
    result = await app(  # type: ignore[func-returns-value]
  File "/usr/local/lib/python3.10/site-packages/uvicorn/middleware/proxy_headers.py", line 78, in __call__
    return await self.app(scope, receive, send)
  File "/usr/local/lib/python3.10/site-packages/fastapi/applications.py", line 276, in __call__
    await super().__call__(scope, receive, send)
  File "/usr/local/lib/python3.10/site-packages/starlette/applications.py", line 122, in __call__
    await self.middleware_stack(scope, receive, send)
  File "/usr/local/lib/python3.10/site-packages/starlette/middleware/errors.py", line 184, in __call__
    raise exc
  File "/usr/local/lib/python3.10/site-packages/starlette/middleware/errors.py", line 162, in __call__
    await self.app(scope, receive, _send)
  File "/usr/local/lib/python3.10/site-packages/timing_asgi/middleware.py", line 68, in __call__
    await self.app(scope, receive, send_wrapper)
  File "/usr/local/lib/python3.10/site-packages/starlette/middleware/exceptions.py", line 79, in __call__
    raise exc
  File "/usr/local/lib/python3.10/site-packages/starlette/middleware/exceptions.py", line 68, in __call__
    await self.app(scope, receive, sender)
  File "/usr/local/lib/python3.10/site-packages/fastapi/middleware/asyncexitstack.py", line 21, in __call__
    raise e
  File "/usr/local/lib/python3.10/site-packages/fastapi/middleware/asyncexitstack.py", line 18, in __call__
    await self.app(scope, receive, send)
  File "/usr/local/lib/python3.10/site-packages/starlette/routing.py", line 718, in __call__
    await route.handle(scope, receive, send)
  File "/usr/local/lib/python3.10/site-packages/starlette/routing.py", line 276, in handle
    await self.app(scope, receive, send)
  File "/usr/local/lib/python3.10/site-packages/starlette/routing.py", line 66, in app
    response = await func(request)
  File "/usr/local/lib/python3.10/site-packages/fastapi/routing.py", line 237, in app
    raw_response = await run_endpoint_function(
  File "/usr/local/lib/python3.10/site-packages/fastapi/routing.py", line 163, in run_endpoint_function
    return await dependant.call(**values)
  File "/usr/local/lib/python3.10/site-packages/kserve/protocol/rest/v2_endpoints.py", line 151, in infer
    response, response_headers = self.dataplane.encode(model_name=model_name,
  File "/usr/local/lib/python3.10/site-packages/kserve/protocol/dataplane.py", line 273, in encode
    response = response.to_rest()
  File "/usr/local/lib/python3.10/site-packages/kserve/protocol/infer_type.py", line 611, in to_rest
    res['parameters'] = to_http_parameters(self.parameters)
  File "/usr/local/lib/python3.10/site-packages/kserve/protocol/infer_type.py", line 688, in to_http_parameters
    parameters[key] = val.string_param
ValueError: Direct assignment of submessage not allowed

python/kserve/test/test_infer_type.py

Signed-off-by: Sivanantham Chinnaiyan <sivanantham.chinnaiyan@ideas2it.com>

sivanantha321 · 2024-01-25T08:42:00Z

Can you try with these changes and give some feedback ?

A gave your branch a go (3a773e7) but I'm hitting an assignment error pasted below. I think I remember hitting the same problem in my workaround and I ended not trying to reassign on the current parameters but instead recreating them.

Traceback (most recent call last):
  File "/usr/local/lib/python3.10/site-packages/uvicorn/protocols/http/httptools_impl.py", line 419, in run_asgi
    result = await app(  # type: ignore[func-returns-value]
  File "/usr/local/lib/python3.10/site-packages/uvicorn/middleware/proxy_headers.py", line 78, in __call__
    return await self.app(scope, receive, send)
  File "/usr/local/lib/python3.10/site-packages/fastapi/applications.py", line 276, in __call__
    await super().__call__(scope, receive, send)
  File "/usr/local/lib/python3.10/site-packages/starlette/applications.py", line 122, in __call__
    await self.middleware_stack(scope, receive, send)
  File "/usr/local/lib/python3.10/site-packages/starlette/middleware/errors.py", line 184, in __call__
    raise exc
  File "/usr/local/lib/python3.10/site-packages/starlette/middleware/errors.py", line 162, in __call__
    await self.app(scope, receive, _send)
  File "/usr/local/lib/python3.10/site-packages/timing_asgi/middleware.py", line 68, in __call__
    await self.app(scope, receive, send_wrapper)
  File "/usr/local/lib/python3.10/site-packages/starlette/middleware/exceptions.py", line 79, in __call__
    raise exc
  File "/usr/local/lib/python3.10/site-packages/starlette/middleware/exceptions.py", line 68, in __call__
    await self.app(scope, receive, sender)
  File "/usr/local/lib/python3.10/site-packages/fastapi/middleware/asyncexitstack.py", line 21, in __call__
    raise e
  File "/usr/local/lib/python3.10/site-packages/fastapi/middleware/asyncexitstack.py", line 18, in __call__
    await self.app(scope, receive, send)
  File "/usr/local/lib/python3.10/site-packages/starlette/routing.py", line 718, in __call__
    await route.handle(scope, receive, send)
  File "/usr/local/lib/python3.10/site-packages/starlette/routing.py", line 276, in handle
    await self.app(scope, receive, send)
  File "/usr/local/lib/python3.10/site-packages/starlette/routing.py", line 66, in app
    response = await func(request)
  File "/usr/local/lib/python3.10/site-packages/fastapi/routing.py", line 237, in app
    raw_response = await run_endpoint_function(
  File "/usr/local/lib/python3.10/site-packages/fastapi/routing.py", line 163, in run_endpoint_function
    return await dependant.call(**values)
  File "/usr/local/lib/python3.10/site-packages/kserve/protocol/rest/v2_endpoints.py", line 151, in infer
    response, response_headers = self.dataplane.encode(model_name=model_name,
  File "/usr/local/lib/python3.10/site-packages/kserve/protocol/dataplane.py", line 273, in encode
    response = response.to_rest()
  File "/usr/local/lib/python3.10/site-packages/kserve/protocol/infer_type.py", line 611, in to_rest
    res['parameters'] = to_http_parameters(self.parameters)
  File "/usr/local/lib/python3.10/site-packages/kserve/protocol/infer_type.py", line 688, in to_http_parameters
    parameters[key] = val.string_param
ValueError: Direct assignment of submessage not allowed

I am not reassigning the parameters dict anymore. Can you retry with these changes ?

Signed-off-by: Sivanantham Chinnaiyan <sivanantham.chinnaiyan@ideas2it.com>

ajstewart · 2024-01-26T10:05:24Z

I am not reassigning the parameters dict anymore. Can you retry with these changes ?

Yep these are being passed through now! The model I am testing is using grpc so once they get to the predictor I have to run the to_http_parameters so I can easily make use of the values in the dict of InferParameter types. But all the functionality is there now to do what I wanted to do, thanks!

yuzisun · 2024-01-26T21:29:29Z

Thanks @sivanantha321 fixing this!

/lgtm
/approve

oss-prow-bot · 2024-01-26T21:29:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sivanantha321, yuzisun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [yuzisun]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* Pass missing infer parameters Signed-off-by: Sivanantham Chinnaiyan <sivanantham.chinnaiyan@ideas2it.com> * Add tests Signed-off-by: Sivanantham Chinnaiyan <sivanantham.chinnaiyan@ideas2it.com> * Fix warnings Signed-off-by: Sivanantham Chinnaiyan <sivanantham.chinnaiyan@ideas2it.com> * Add equal magic method for infer types Signed-off-by: Sivanantham Chinnaiyan <sivanantham.chinnaiyan@ideas2it.com> * Avoid inplace modification of parameters in conversion Signed-off-by: Sivanantham Chinnaiyan <sivanantham.chinnaiyan@ideas2it.com> --------- Signed-off-by: Sivanantham Chinnaiyan <sivanantham.chinnaiyan@ideas2it.com>

oss-prow-bot bot requested review from adriangonz and rachitchauhan43 January 16, 2024 12:06

sivanantha321 force-pushed the pass-infer-parameters branch 2 times, most recently from db56d96 to 4f9852c Compare January 18, 2024 09:17

sivanantha321 force-pushed the pass-infer-parameters branch from 4f9852c to e55e02c Compare January 19, 2024 05:34

sivanantha321 marked this pull request as draft January 19, 2024 06:29

oss-prow-bot bot added the do-not-merge/work-in-progress label Jan 19, 2024

sivanantha321 marked this pull request as ready for review January 19, 2024 07:59

oss-prow-bot bot removed the do-not-merge/work-in-progress label Jan 19, 2024

oss-prow-bot bot requested review from lizzzcai and theofpa January 19, 2024 07:59

sivanantha321 force-pushed the pass-infer-parameters branch from e55e02c to 9efd39b Compare January 19, 2024 08:08

yuzisun reviewed Jan 22, 2024

View reviewed changes

python/kserve/kserve/protocol/infer_type.py Outdated Show resolved Hide resolved

sivanantha321 force-pushed the pass-infer-parameters branch 3 times, most recently from cbba47d to 3a773e7 Compare January 24, 2024 08:33

sivanantha321 commented Jan 24, 2024

View reviewed changes

yuzisun reviewed Jan 24, 2024

View reviewed changes

python/kserve/test/test_infer_type.py Outdated Show resolved Hide resolved

yuzisun reviewed Jan 24, 2024

View reviewed changes

python/kserve/test/test_infer_type.py Outdated Show resolved Hide resolved

yuzisun reviewed Jan 24, 2024

View reviewed changes

python/kserve/test/test_infer_type.py Outdated Show resolved Hide resolved

sivanantha321 added 4 commits January 25, 2024 14:08

Pass missing infer parameters

c9d2f7d

Signed-off-by: Sivanantham Chinnaiyan <sivanantham.chinnaiyan@ideas2it.com>

Add tests

98e92e9

Signed-off-by: Sivanantham Chinnaiyan <sivanantham.chinnaiyan@ideas2it.com>

Fix warnings

718fcab

Signed-off-by: Sivanantham Chinnaiyan <sivanantham.chinnaiyan@ideas2it.com>

Add equal magic method for infer types

4c9a5f1

Signed-off-by: Sivanantham Chinnaiyan <sivanantham.chinnaiyan@ideas2it.com>

sivanantha321 force-pushed the pass-infer-parameters branch from 3a773e7 to 5eff780 Compare January 25, 2024 08:39

Avoid inplace modification of parameters in conversion

d9a955c

Signed-off-by: Sivanantham Chinnaiyan <sivanantham.chinnaiyan@ideas2it.com>

sivanantha321 force-pushed the pass-infer-parameters branch from 5eff780 to d9a955c Compare January 25, 2024 08:48

oss-prow-bot bot assigned yuzisun Jan 26, 2024

oss-prow-bot bot added the lgtm label Jan 26, 2024

oss-prow-bot bot added the approved label Jan 26, 2024

oss-prow-bot bot merged commit 1bfba02 into kserve:master Jan 26, 2024
58 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass missing infer parameters during conversion #3368

Pass missing infer parameters during conversion #3368

sivanantha321 commented Jan 16, 2024 •

edited

Loading

ajstewart commented Jan 18, 2024

sivanantha321 commented Jan 19, 2024

yuzisun commented Jan 22, 2024

sivanantha321 commented Jan 24, 2024

sivanantha321 Jan 24, 2024

ajstewart commented Jan 24, 2024 •

edited

Loading

sivanantha321 commented Jan 25, 2024

ajstewart commented Jan 26, 2024

yuzisun commented Jan 26, 2024

oss-prow-bot bot commented Jan 26, 2024

Pass missing infer parameters during conversion #3368

Pass missing infer parameters during conversion #3368

Conversation

sivanantha321 commented Jan 16, 2024 • edited Loading

ajstewart commented Jan 18, 2024

sivanantha321 commented Jan 19, 2024

yuzisun commented Jan 22, 2024

sivanantha321 commented Jan 24, 2024

sivanantha321 Jan 24, 2024

Choose a reason for hiding this comment

ajstewart commented Jan 24, 2024 • edited Loading

sivanantha321 commented Jan 25, 2024

ajstewart commented Jan 26, 2024

yuzisun commented Jan 26, 2024

oss-prow-bot bot commented Jan 26, 2024

sivanantha321 commented Jan 16, 2024 •

edited

Loading

ajstewart commented Jan 24, 2024 •

edited

Loading