Skip to content

Local response cache implementation#1440

Merged
adriangonz merged 11 commits into
SeldonIO:masterfrom
SachinVarghese:response_cache
Oct 26, 2023
Merged

Local response cache implementation#1440
adriangonz merged 11 commits into
SeldonIO:masterfrom
SachinVarghese:response_cache

Conversation

@SachinVarghese

@SachinVarghese SachinVarghese commented Oct 15, 2023

Copy link
Copy Markdown
Contributor

This PR introduces basic local response caching as per the issue #1023
The implementation is inspired by the triton equivalent local cache https://github.com/triton-inference-server/local_cache

Signed-off-by: Sachin Varghese <sachin.mathew31@gmail.com>
Signed-off-by: Sachin Varghese <sachin.mathew31@gmail.com>
Signed-off-by: Sachin Varghese <sachin.mathew31@gmail.com>
@SachinVarghese

Copy link
Copy Markdown
Contributor Author

PTAL @adriangonz 🙂

@SachinVarghese SachinVarghese changed the title Response cache Local response cache implementation Oct 15, 2023
Signed-off-by: Sachin Varghese <sachin.mathew31@gmail.com>
Signed-off-by: Sachin Varghese <sachin.mathew31@gmail.com>
Signed-off-by: Sachin Varghese <sachin.mathew31@gmail.com>
Signed-off-by: Sachin Varghese <sachin.mathew31@gmail.com>

@adriangonz adriangonz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one @SachinVarghese! It's great to see you back contributing some good stuff to MLServer!

I've added a few comments, but would be good to hear your thoughts on those. Besides that, it would also be great if you could add some tests around the caching behaviour.

Comment thread mlserver/cache/local/local.py
Comment thread mlserver/handlers/dataplane.py Outdated
Comment thread mlserver/handlers/dataplane.py
Comment thread mlserver/handlers/dataplane.py Outdated
Comment thread mlserver/server.py Outdated
Comment thread mlserver/settings.py Outdated
Comment thread mlserver/settings.py Outdated
Signed-off-by: Sachin Varghese <sachin.mathew31@gmail.com>
Comment thread mlserver/settings.py Outdated
Signed-off-by: Sachin Varghese <sachin.mathew31@gmail.com>
@SachinVarghese

Copy link
Copy Markdown
Contributor Author

I have made most of the suggested changes now, and all the CI tests should be passing. Please review.
Also, I'm not fully sure how to write the tests for something like this. Any ideas would be good.

@adriangonz

Copy link
Copy Markdown
Contributor

I think for this one we probably want to add two types of tests:

  • Unit tests for the cache itself, which could go under tests/cache/.... These can test the LocalCache implementation, i.e. does it rotate values correctly when it's full, what happens when key is not present, does it save the values, etc.
  • Integration tests for the DataPlane class, which can go under the tests/handlers/.... These can test that the DataPlane class create the cache correctly, picks up on cached values, caches new responses, ignores the cache when disabled, etc.

@SachinVarghese SachinVarghese force-pushed the response_cache branch 2 times, most recently from 67174eb to 7d865c0 Compare October 20, 2023 17:29
Signed-off-by: Sachin Varghese <sachin.mathew31@gmail.com>
Signed-off-by: Sachin Varghese <sachin.mathew31@gmail.com>
@SachinVarghese

Copy link
Copy Markdown
Contributor Author

I have added a few tests for the local cache and data plane component now, please review.

@adriangonz adriangonz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one @SachinVarghese ! Thanks for adding those changes in.

This should be good to go from my side. 👍

@adriangonz adriangonz requested a review from saeid93 October 23, 2023 08:52
@SachinVarghese

Copy link
Copy Markdown
Contributor Author

Awesome and thanks for reviewing/ almost co-authoring 😄 Will wait for other reviews.

@adriangonz adriangonz merged commit a94068b into SeldonIO:master Oct 26, 2023
@adriangonz adriangonz mentioned this pull request Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants