feat: Pulling in dependencies (in_process mode) using conda environment #4807
Conversation
|
|
||
|
|
||
| class RequirementsManager: | ||
| """Transformers build logic with ModelBuilder()""" |
| """Transformers build logic with ModelBuilder()""" | ||
|
|
||
| def detect_file_exists(self, dependencies: str = None) -> str: | ||
| """Creates snapshot of the user's environment |
There was a problem hiding this comment.
Nit: Not sure if this is entirely right? we are detecting not snapshotting anything?
There was a problem hiding this comment.
Yes that is right, good catch, I will rephrase.
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| RequirementsManager().detect_file_exists(dependencies="auto_capture") |
There was a problem hiding this comment.
How is auto_capture as a value used?
There was a problem hiding this comment.
It ended up being unused so I have removed it.
| @@ -227,6 +234,22 @@ def _transformers_model_builder_deploy_wrapper(self, *args, **kwargs) -> Type[Pr | |||
| ) | |||
| return predictor | |||
|
|
|||
| if self.mode == Mode.IN_PROCESS: | |||
There was a problem hiding this comment.
It will be removed.
| @patch( | ||
| "sagemaker.serve.builder.requirements_manager.RequirementsManager._capture_from_local_runtime" | ||
| ) | ||
| def test_detect_file_exists_fail(self, mock__capture_from_local_runtime) -> str: |
| mock_sample_output = [{"generated_text": mock_response}] | ||
|
|
||
|
|
||
| class TestInProcessMode(unittest.TestCase): |
There was a problem hiding this comment.
I assume this will be removed on revision
There was a problem hiding this comment.
Yes, I will be removing in_process mode changes as that is a different PR: #4784
|
|
||
| # Dependencies specified as either req.txt or conda_env.yml | ||
| if dependencies.endswith(".txt"): | ||
| self._install_requirements_txt() |
There was a problem hiding this comment.
I think you're doing multiple things in one function. It's generally recommended to have functions as single tenant. Also, if you want to install while check path existence, you probably want to change the naming as well.
There was a problem hiding this comment.
My naming was making the functions confusing to understand, I will rename so they better fit their purpose. Thank you for pointing this out.
| """Returns the conda prefix from the set environment variable. None otherwise.""" | ||
| return os.getenv("CONDA_PREFIX") | ||
|
|
||
| def _capture_from_local_runtime(self) -> str: |
There was a problem hiding this comment.
Maybe I missed something here, but isn't this function just to generate an empty file? Can you point me to where dependencies are actually captured?
There was a problem hiding this comment.
I will be making a new .txt file similar to the .yml one with required dependencies, good catch, thank you.
| dependencies = self._detect_conda_env_and_local_dependencies() | ||
|
|
||
| # Dependencies specified as either req.txt or conda_env.yml | ||
| if dependencies.endswith(".txt"): |
There was a problem hiding this comment.
_detect_conda_env_and_local_dependencies() will either return conda_in_process.yml or raise ValueError("No conda environment seems to be active.") , in other words, if dependencies.endswith(".txt") will never evaluate to true
There was a problem hiding this comment.
This was a good find. Thanks for your help with modifying it!
| class RequirementsManager: | ||
| """Manages dependency installation by detecting file types""" | ||
|
|
||
| def capture_and_install_dependencies(self, dependencies: str = None) -> str: |
There was a problem hiding this comment.
input dependencies will always be shadowed since there's a local variable named as the same name
Issue #, if available:
Description of changes:
Added a new class (RequirementsManager) that detects if a .txt or .yml file provided for dependencies, and installs them accordingly via pip or conda env.
Testing done:
Integr tests run locally.
Merge Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.General
Tests
unique_name_from_baseto create resource names in integ tests (if appropriate)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.