vandonr-amz commented on code in PR #32112:
URL: https://github.com/apache/airflow/pull/32112#discussion_r1240552004
##########
tests/system/providers/amazon/aws/example_sagemaker.py:
##########
@@ -159,12 +159,11 @@ def _build_and_upload_docker_image(preprocess_script,
repository_uri):
docker_build_and_push_commands = f"""
cp /root/.aws/credentials /tmp/credentials &&
# login to public ecr repo containing amazonlinux image
- docker login --username {creds.username} --password
{creds.password} public.ecr.aws
+ docker login --username {creds.username} --password
{creds.password} public.ecr.aws &&
Review Comment:
the old version was working too, but at least this homogeneous with the
other lines of the command
##########
tests/system/providers/amazon/aws/example_sagemaker.py:
##########
@@ -178,7 +177,8 @@ def _build_and_upload_docker_image(preprocess_script,
repository_uri):
if docker_build.returncode != 0:
raise RuntimeError(
"Failed to prepare docker image for the preprocessing job.\n"
- f"The following error happened while executing the sequence of
bash commands:\n{stderr}"
+ "The following error happened while executing the sequence of
bash commands:\n"
+ f"{stderr.decode()}"
Review Comment:
stderr is a byte array. Without the `decode()`, the `\n` are not escaped for
instance, and the whole output appears on one line, which is _not the best_
##########
tests/system/providers/amazon/aws/example_sagemaker.py:
##########
@@ -159,12 +159,11 @@ def _build_and_upload_docker_image(preprocess_script,
repository_uri):
docker_build_and_push_commands = f"""
cp /root/.aws/credentials /tmp/credentials &&
# login to public ecr repo containing amazonlinux image
- docker login --username {creds.username} --password
{creds.password} public.ecr.aws
+ docker login --username {creds.username} --password
{creds.password} public.ecr.aws &&
docker build --platform=linux/amd64 -f {dockerfile.name} -t
{repository_uri} /tmp &&
rm /tmp/credentials &&
# login again, this time to the private repo we created to hold
that specific image
- aws ecr get-login-password --region {ecr_region} |
Review Comment:
this is probably from before, when we didn't pass credentials in the command
line, but it's now useless.
##########
airflow/providers/amazon/aws/triggers/sagemaker.py:
##########
@@ -41,7 +42,7 @@ def __init__(
job_name: str,
job_type: str,
poke_interval: int = 30,
- max_attempts: int | None = None,
+ max_attempts: int = 480,
Review Comment:
passing None was not working before, so this is not a breaking change since
it was already broken.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
