Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: remove historical job_name caching which causes long job name #5118

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+1 −44
Open

fix: remove historical job_name caching which causes long job name #5118

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1bef225
fix: remove historical job_name caching which causes job names to exc…
Apr 3, 2025
867743c
fix: remove unused urlparse import to address flake8 failure
Apr 4, 2025
2c7388c
Merge branch 'master' into master
jkasiraj Apr 4, 2025
b7bfa3a
fix: set job_name to None when initializing TrainingStep
Apr 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 1 addition & 44 deletions src/sagemaker/workflow/steps.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

from enum import Enum
from typing import Dict, List, Set, Union, Optional, Any, TYPE_CHECKING
from urllib.parse import urlparse

import attr

Expand Down Expand Up @@ -465,6 +464,7 @@ def __init__(
self.step_args = step_args
self.estimator = estimator
self.inputs = inputs
self.job_name = None

self._properties = Properties(
step_name=name, step=self, shape_name="DescribeTrainingJobResponse"
Expand Down Expand Up @@ -493,19 +493,6 @@ def __init__(
DeprecationWarning,
)

self.job_name = None
Copy link
Contributor

@benieric benieric Apr 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Can we generate the name using the name of the image? That is what other interfaces do. Does removing these code blocks break anything? Maybe need a closer look but if 'job_name = None' we may need to generate a name some other way no?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/aws/sagemaker-python-sdk/blob/master/src/sagemaker/pipeline.py#L244
https://github.com/aws/sagemaker-python-sdk/blob/master/src/sagemaker/modules/train/model_trainer.py#L452

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I think we can set it to self.name, since the removed code was setting it to f"{self.name}-{code_hash}"[:1024]

I believe right now the behavior is that if an estimator is set in a particular way (logic from lines 497-506) it leads to a job name that's too long + errors or otherwise it defaults to None, which is why I considered leaving it as None

I'm a bit hesitant to add new behavior like trying to parse the image (which technically isn't required by the api)

Copy link
Contributor Author

@jkasiraj jkasiraj Apr 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, I looked into it further and the job name will eventually get generated here if it doesn't exist so I don't think it needs to be set in this function

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotchu sounds good 👍

if estimator and (estimator.source_dir or estimator.entry_point):
# By default, `Estimator` will upload the local code to an S3 path
# containing a timestamp. This causes cache misses whenever a
# pipeline is updated, even if the underlying script hasn't changed.
# To avoid this, hash the contents of the training script and include it
# in the `job_name` passed to the `Estimator`, which will be used
# instead of the timestamped path.
if not is_pipeline_variable(estimator.source_dir) and not is_pipeline_variable(
estimator.entry_point
):
self.job_name = self._generate_code_upload_path()

@property
def arguments(self) -> RequestType:
"""The arguments dictionary that is used to call `create_training_job`.
Expand Down Expand Up @@ -554,26 +541,6 @@ def to_request(self) -> RequestType:

return request_dict

def _generate_code_upload_path(self) -> str or None:
"""Generate an upload path for local training scripts based on their content."""
from sagemaker.workflow.utilities import hash_files_or_dirs

if self.estimator.source_dir:
source_dir_url = urlparse(self.estimator.source_dir)
if source_dir_url.scheme == "" or source_dir_url.scheme == "file":
code_hash = hash_files_or_dirs(
[self.estimator.source_dir] + self.estimator.dependencies
)
return f"{self.name}-{code_hash}"[:1024]
elif self.estimator.entry_point:
entry_point_url = urlparse(self.estimator.entry_point)
if entry_point_url.scheme == "" or entry_point_url.scheme == "file":
code_hash = hash_files_or_dirs(
[self.estimator.entry_point] + self.estimator.dependencies
)
return f"{self.name}-{code_hash}"[:1024]
return None


class CreateModelStep(ConfigurableRetryStep):
"""`CreateModelStep` for SageMaker Pipelines Workflows."""
Expand Down Expand Up @@ -895,16 +862,6 @@ def __init__(
"code argument has to be a valid S3 URI or local file path "
+ "rather than a pipeline variable"
)
code_url = urlparse(code)
if code_url.scheme == "" or code_url.scheme == "file":
# By default, `Processor` will upload the local code to an S3 path
# containing a timestamp. This causes cache misses whenever a
# pipeline is updated, even if the underlying script hasn't changed.
# To avoid this, hash the contents of the script and include it
# in the `job_name` passed to the `Processor`, which will be used
# instead of the timestamped path.
self.job_name = self._generate_code_upload_path()

warnings.warn(
(
'We are deprecating the instantiation of ProcessingStep using "processor".'
Expand Down
Loading