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

Feature/support existing environments #1891

Merged

Conversation

idlefella
Copy link
Contributor

@idlefella idlefella commented Aug 22, 2024

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

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

Additional Notes

@CLAassistant
Copy link

CLAassistant commented Aug 22, 2024

CLA assistant check
All committers have signed the CLA.

@idlefella idlefella force-pushed the feature/support-existing-environments branch from 7e1aa73 to 8471a49 Compare August 22, 2024 09:07
@sakoush sakoush self-requested a review September 2, 2024 20:10
Copy link
Member

@sakoush sakoush left a comment

Choose a reason for hiding this comment

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

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?

mlserver/env.py Outdated Show resolved Hide resolved
mlserver/parallel/registry.py Outdated Show resolved Hide resolved
@@ -277,6 +278,21 @@ Note that, in the folder layout above, we are assuming that:
}
```

If you want to use an already exisiting python environment, you can use the parameter `environment_path` of your `model-settings.json`:
Copy link
Member

Choose a reason for hiding this comment

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

maybe add in the docs that we can have envs etc. in the enviornment_path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you explain that further? I'm not quite sure what I'm supposed to do.

Copy link
Member

Choose a reason for hiding this comment

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

@ramonpzg minor changes to the docs FYI

mlserver/parallel/registry.py Outdated Show resolved Hide resolved
@idlefella
Copy link
Contributor Author

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

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

@sakoush
Copy link
Member

sakoush commented Sep 4, 2024

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

@idlefella
Copy link
Contributor Author

@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

@sakoush sakoush merged commit 3531940 into SeldonIO:master Sep 4, 2024
26 checks passed
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.

3 participants