Skip to content

Feature/support existing environments#1891

Merged
sakoush merged 8 commits into
SeldonIO:masterfrom
idlefella:feature/support-existing-environments
Sep 4, 2024
Merged

Feature/support existing environments#1891
sakoush merged 8 commits into
SeldonIO:masterfrom
idlefella:feature/support-existing-environments

Conversation

@idlefella

@idlefella idlefella commented Aug 22, 2024

Copy link
Copy Markdown
Contributor

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

CLAassistant commented Aug 22, 2024

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@sakoush
sakoush self-requested a review September 2, 2024 20:10

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

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?

Comment thread mlserver/env.py Outdated
Comment thread mlserver/parallel/registry.py Outdated
Comment thread docs/user-guide/custom.md
}
```

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

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.

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

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

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

Comment thread mlserver/parallel/registry.py Outdated
@idlefella

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

sakoush commented Sep 4, 2024

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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
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