You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "vandonr-amz (via GitHub)" <gi...@apache.org> on 2023/06/24 00:50:06 UTC
[GitHub] [airflow] vandonr-amz opened a new pull request, #32112: deferrable mode for `SageMakerTuningOperator`
vandonr-amz opened a new pull request, #32112:
URL: https://github.com/apache/airflow/pull/32112
took the opportunity to fix tiny things in the system test,
and also migrate to the trigger to the method that logs the status while it waits
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vandonr-amz commented on a diff in pull request #32112: deferrable mode for `SageMakerTuningOperator`
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vandonr-amz commented on pull request #32112: deferrable mode for `SageMakerTuningOperator` and `SageMakerEndpointOperator`
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on PR #32112:
URL: https://github.com/apache/airflow/pull/32112#issuecomment-1608215922
TIL: when updating a PR title with `markdown`, you need to refresh the page to see it formatted
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #32112: deferrable mode for `SageMakerTuningOperator` and `SageMakerEndpointOperator`
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #32112:
URL: https://github.com/apache/airflow/pull/32112
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org