-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix(tgi): honor S3 model_path as weight source for TGI builds (#5943) #5964
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,7 @@ | |
| # SageMaker core imports | ||
| from sagemaker.core.resources import Model, Endpoint | ||
| from sagemaker.core.utils.utils import logger | ||
| from sagemaker.core.common_utils import _is_s3_uri | ||
|
|
||
|
|
||
| # SageMaker serve imports | ||
|
|
@@ -220,11 +221,25 @@ def _build_for_tgi(self) -> Model: | |
|
|
||
| from sagemaker.serve.model_server.tgi.prepare import _create_dir_structure | ||
|
|
||
| _create_dir_structure(self.model_path) | ||
| # Detect an S3 weight source from model_path before any local directory is | ||
| # created. TGI's HF_MODEL_ID does not accept an S3 URI, so an S3 source is | ||
| # attached as an uncompressed ModelDataSource (mounted at /opt/ml/model) | ||
| # instead of being downloaded from the HuggingFace Hub. | ||
| s3_model_source = self.model_path if _is_s3_uri(self.model_path) else None | ||
|
|
||
| # Skip the local mkdir for an S3 source so we do not create a literal | ||
| # local "s3:/..." directory tree; only create it for genuine local paths. | ||
| if not s3_model_source: | ||
| _create_dir_structure(self.model_path) | ||
|
|
||
| if isinstance(self.model, str) and not self._is_jumpstart_model_id(): | ||
| # Configure HuggingFace model for TGI | ||
| self.env_vars.setdefault("HF_MODEL_ID", self.model) | ||
| if s3_model_source: | ||
| # Weights are mounted at /opt/ml/model; do not download from the Hub. | ||
| self.env_vars.setdefault("HF_MODEL_ID", "/opt/ml/model") | ||
| self.env_vars.setdefault("HF_HUB_OFFLINE", "1") | ||
| else: | ||
| self.env_vars.setdefault("HF_MODEL_ID", self.model) | ||
|
|
||
| self.hf_model_config = _get_model_config_properties_from_hf( | ||
| self.model, self.env_vars.get("HUGGING_FACE_HUB_TOKEN") | ||
|
|
@@ -267,6 +282,11 @@ def _build_for_tgi(self) -> Model: | |
| if not self._optimizing: | ||
| if self.mode in LOCAL_MODES: | ||
| self._prepare_for_mode(should_upload_artifacts=True) | ||
| elif s3_model_source: | ||
| # Route the S3 weight source through _prepare_for_mode so the | ||
| # _upload_tgi_artifacts S3 branch builds the uncompressed | ||
| # ModelDataSource (CompressionType="None", S3DataType="S3Prefix"). | ||
| self.s3_model_data_url, _ = self._prepare_for_mode(model_path=s3_model_source) | ||
| else: | ||
| self.s3_model_data_url, _ = self._prepare_for_mode() | ||
|
|
||
|
|
@@ -299,7 +319,10 @@ def _build_for_tgi(self) -> Model: | |
|
|
||
| model = self._create_model() | ||
|
|
||
| if "HF_HUB_OFFLINE" in self.env_vars: | ||
| # Reset the in-memory HF_HUB_OFFLINE flag after the container is built, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need to reset?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an existing logic (which is if this var is set, we must be in a local build, so reset it for cleanliness). |
||
| # EXCEPT when weights are mounted from S3: those must stay offline so TGI | ||
| # loads from /opt/ml/model instead of phoning home to the HuggingFace Hub. | ||
| if "HF_HUB_OFFLINE" in self.env_vars and not s3_model_source: | ||
| self.env_vars.update({"HF_HUB_OFFLINE": "0"}) | ||
|
|
||
| return model | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting HF_MODEL_ID to /opt/ml/model is a sane default when s3_model_source is used.
But HF_HUB_OFFLINE, this may not need to be set for fetching model weights from s3. If customers want to explicitly set it, they should do so by passing env_vars when ModelBuilder is created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a design decision that follows the pattern here https://github.com/aws/sagemaker-python-sdk/blob/master/sagemaker-serve/src/sagemaker/serve/model_builder_servers.py#L274
The idea is that if weights are on local disk, don't go to the Hub. S3 case is semantically same (weights on disk at /opt/ml/model). Setting it keeps that intent consistent.
Also if the s3 url is incorrect, it would silently fall back to downloading it from HF as HF lib will assume that it is a HF link. The container will stand up after 10 mins with no errors with no obvious indication that S3 link didnt work. The offline mode would surface the error immediately. So I suggest keeping this pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good reasoning. Will test this out as well.