MLServer icon indicating copy to clipboard operation
MLServer copied to clipboard

Published 20 hours ago • verified publisher icon SeldonIO

Feature/support existing environments

Open idlefella opened this issue 1 year ago • 1 comments
trafficstars

Using Pre-existing Python Environments

Description

The current implementation of mlserver only supports specifying a Python environment through a tarball, which is then unpacked before the workers are activated. Our configuration, however, already includes pre-defined environments we aim to utilize. The current pull request offers an option to select a path to an already set up Python environment.

Changes Made

  • Added the environment_path model parameter to specify the path to the existing environment.
  • Relocated the code responsible for removing an unpacked environment from PoolRegistry to Environment – the component where it’s unpacked
  • Integrated a delete_env attribute within Environment, determining whether the environment should be deleted from the disk upon the application's termination.
  • Added documentation how to use it

Related Issues

Screenshots (if applicable)

Checklist

  • [x] Code follows the project's style guidelines
  • [x] All tests related to the changes pass successfully
  • [x] Documentation is updated (if necessary)
  • [ ] Code is reviewed by at least one other team member
  • [x] Any breaking changes are communicated and documented

Additional Notes

idlefella avatar Aug 22 '24 07:08 idlefella

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Aug 22 '24 07:08 CLAassistant

Many thanks @idlefella for your contribution. This makes sense.

The logic seems ok, left some minor comments.

This change lacks though test coverage, could you please add relevant test cases for the new code?

Hi @sakoush Thanks for your comments. I will resolve the issues asap, hope to find time today.

idlefella avatar Sep 03 '24 07:09 idlefella

This change lacks though test coverage, could you please add relevant test cases for the new code?

Hi @sakoush, I've added unittests for the code. Let me know if there's anything else to do

idlefella avatar Sep 03 '24 14:09 idlefella

@idlefella I have triggered CI and it looks like there are some linting issue. Could you take a look? you can also run lint locally using make fmt / make lint. Otherwise should be good to go.

sakoush avatar Sep 04 '24 13:09 sakoush

@idlefella I have triggered CI and it looks like there are some linting issue. Could you take a look? you can also run lint locally using make fmt / make lint. Otherwise should be good to go.

Hi @sakoush Thanks for running the CI. I've fixed the formatting of the code. Best

idlefella avatar Sep 04 '24 13:09 idlefella