You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/02/24 22:58:23 UTC
[GitHub] [airflow] ferruzzi opened a new issue #21808: Add default 'aws_conn_id' to SageMaker Operators
ferruzzi opened a new issue #21808:
URL: https://github.com/apache/airflow/issues/21808
The SageMaker Operators not having a default value for `aws_conn_id` is a pain, we should fix that. See EKS operators for an example: https://github.com/apache/airflow/blob/main/airflow/providers/amazon/aws/operators/eks.py
_Originally posted by @ferruzzi in https://github.com/apache/airflow/pull/21673#discussion_r813414043_
--
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] hsrocks commented on issue #21808: Add default 'aws_conn_id' to SageMaker Operators
Posted by GitBox <gi...@apache.org>.
hsrocks commented on issue #21808:
URL: https://github.com/apache/airflow/issues/21808#issuecomment-1058802986
Hello @ferruzzi :)
I reviewed the code for changes and as per me these changes will not be needed.
As part of Sagemaker Base : https://github.com/apache/airflow/blob/main/airflow/providers/amazon/aws/operators/sagemaker.py#L51
the connection id is assigned a default value . Let me know if anything needs to be improvise. Thanks!!
--
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] ferruzzi commented on issue #21808: Add default 'aws_conn_id' to SageMaker Operators
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on issue #21808:
URL: https://github.com/apache/airflow/issues/21808#issuecomment-1058924049
The problem is that the individual operators also declare the aws_conn without a default value so it isn't optional, and it won't fall back to that value:
```
In [1]: class Foo():
...: def __init__(self, myvar=5):
...: self.myvar = myvar
...:
...: def op(self):
...: return self.myvar
...:
...: class Bar(Foo):
...: def __init__(self, myvar):
...: super().__init__(myvar=myvar)
...:
In [2]: Foo().op()
Out[2]: 5
In [3]: Foo(3).op()
Out[3]: 3
In [4]: Bar(2).op()
Out[4]: 2
In [5]: Bar().op()
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
<ipython-input-6-06b11a1ebf4e> in <module>
----> 1 Bar().op()
TypeError: __init__() missing 1 required positional argument: 'myvar'
```
So if we move the defaulting into the child Operators, we don't have to declare `aws_conn_id='default_aws'` in every operator use:
```
In [1]: class Foo:
...: def __init__(self):
...: pass
...:
...: def op(self):
...: return self.myvar
...:
...:
...: class Bar(Foo):
...: def __init__(self, myvar=5):
...: self.myvar = myvar
...: super().__init__()
...:
In [2]: Bar(2).op()
Out[2]: 2
In [3]: Bar().op()
Out[3]: 5
```
--
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] eladkal commented on issue #21808: Add default 'aws_conn_id' to SageMaker Operators
Posted by GitBox <gi...@apache.org>.
eladkal commented on issue #21808:
URL: https://github.com/apache/airflow/issues/21808#issuecomment-1053539866
@hsrocks would you like to work on the followup task?
--
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] hsrocks commented on issue #21808: Add default 'aws_conn_id' to SageMaker Operators
Posted by GitBox <gi...@apache.org>.
hsrocks commented on issue #21808:
URL: https://github.com/apache/airflow/issues/21808#issuecomment-1059029976
Got the intent . Yes can see this problem with few of them individually . Appreciate the details :) Will work this out.!!
--
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] hsrocks commented on issue #21808: Add default 'aws_conn_id' to SageMaker Operators
Posted by GitBox <gi...@apache.org>.
hsrocks commented on issue #21808:
URL: https://github.com/apache/airflow/issues/21808#issuecomment-1053558166
> @hsrocks would you like to work on the followup task?
Thanks @eladkal please assign it to me. Will do
--
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 commented on issue #21808: Add default 'aws_conn_id' to SageMaker Operators
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #21808:
URL: https://github.com/apache/airflow/issues/21808#issuecomment-1053743312
Assigned!
--
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] ferruzzi commented on issue #21808: Add default 'aws_conn_id' to SageMaker Operators
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on issue #21808:
URL: https://github.com/apache/airflow/issues/21808#issuecomment-1054504752
Hey, cool. Thanks for taking it on.
--
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