Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Local response cache implementation #1440

Merged
merged 11 commits into from
Oct 26, 2023

Conversation

SachinVarghese
Copy link
Contributor

@SachinVarghese SachinVarghese commented Oct 15, 2023

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
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>
Copy link
Contributor

@adriangonz adriangonz left a comment

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.

mlserver/cache/local/local.py Show resolved Hide resolved
mlserver/handlers/dataplane.py Outdated Show resolved Hide resolved
mlserver/handlers/dataplane.py Show resolved Hide resolved
mlserver/handlers/dataplane.py Outdated Show resolved Hide resolved
mlserver/server.py Outdated Show resolved Hide resolved
mlserver/settings.py Outdated Show resolved Hide resolved
mlserver/settings.py Outdated Show resolved Hide resolved
Signed-off-by: Sachin Varghese <sachin.mathew31@gmail.com>
mlserver/settings.py Outdated Show resolved Hide resolved
Signed-off-by: Sachin Varghese <sachin.mathew31@gmail.com>
@SachinVarghese
Copy link
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
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
Contributor Author

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

Copy link
Contributor

@adriangonz adriangonz left a comment

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. 👍

@SachinVarghese
Copy link
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
30 checks passed
@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