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 2023/01/09 21:43:36 UTC
[GitHub] [airflow] vandonr-amz opened a new pull request, #28816: introduce a method to convert dictionaries to boto-style key-value lists
vandonr-amz opened a new pull request, #28816:
URL: https://github.com/apache/airflow/pull/28816
Boto expects key/value lists in a weird format that's not super user friendly. I think we should offer a pleasant interface to the users, so we should accept regular python dictionaries and do the transformation ourselves when possible. I added a method to this effect, and plugged it in where I could see this pattern.
--
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] Taragolis commented on pull request #28816: introduce a method to convert dictionaries to boto-style key-value lists
Posted by GitBox <gi...@apache.org>.
Taragolis commented on PR #28816:
URL: https://github.com/apache/airflow/pull/28816#issuecomment-1377103727
Also I think we need to keep compatibility with native API parts. I mean it would be nice if function check (btw i think you forget to add it into the PR) if value are dictionary then convert it to appropriate format, if not.. well then return as is or check if this has appropriate format.
Actually I also work on the same issue in the past, I don't remember when I didn't finish it, may be because I could't cover all cases or something like that. I've prepare [GIST](https://gist.github.com/Taragolis/2c77ff7fa5337659ac44c303c3af1160) based on some past findings, it is not concern that we should do by this way, just an example. It use additional ability of Airflow render particular attributes from [inner object](https://airflow.apache.org/docs/apache-airflow/stable/concepts/operators.html#jinja-templating)
--
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 #28816: introduce a method to convert dictionaries to boto-style key-value lists
Posted by GitBox <gi...@apache.org>.
vandonr-amz commented on PR #28816:
URL: https://github.com/apache/airflow/pull/28816#issuecomment-1397776181
@eladkal @uranusjr anything else you'd like changed with that PR ? Otherwise we could merge it 😇
--
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] uranusjr commented on a diff in pull request #28816: introduce a method to convert dictionaries to boto-style key-value lists
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #28816:
URL: https://github.com/apache/airflow/pull/28816#discussion_r1070949997
##########
airflow/providers/amazon/aws/hooks/s3.py:
##########
@@ -991,33 +992,40 @@ def get_bucket_tagging(self, bucket_name: str | None = None) -> list[dict[str, s
@provide_bucket_name
def put_bucket_tagging(
self,
- tag_set: list[dict[str, str]] | None = None,
+ tag_set: dict | list[dict[str, str]] | None = None,
Review Comment:
```suggestion
tag_set: dict[str, str] | list[dict[str, str]] | None = None,
```
?
--
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 a diff in pull request #28816: introduce a method to convert dictionaries to boto-style key-value lists
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #28816:
URL: https://github.com/apache/airflow/pull/28816#discussion_r1071472110
##########
airflow/providers/amazon/aws/hooks/s3.py:
##########
@@ -42,6 +42,7 @@
from airflow.exceptions import AirflowException
from airflow.providers.amazon.aws.exceptions import S3HookUriParseFailure
from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
+from airflow.providers.amazon.aws.utils.aws_api import format_tags
Review Comment:
I'm not sure what else you;'d name the module here that would make more sense? It cold be boto_api or boto_utils maybe but I'm not sure either of those are any better, really?
--
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] Taragolis commented on pull request #28816: introduce a method to convert dictionaries to boto-style key-value lists
Posted by GitBox <gi...@apache.org>.
Taragolis commented on PR #28816:
URL: https://github.com/apache/airflow/pull/28816#issuecomment-1376980445
> However, I don't really see why you'd want to template dictionary keys. Templating values makes sense, but keys should be pretty much constant
I don't. My opinion that AWS resource Tag name (key) should be consistent or all idea would be broken and turned into the mess.
However we don't know user cases, so at least we should add info that behaviour with dict-style could be different rather then use native AWS API style (this is not boto3 behaviour this how what AWS API expected this parameters)
--
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 #28816: introduce a method to convert dictionaries to boto-style key-value lists
Posted by GitBox <gi...@apache.org>.
vandonr-amz commented on PR #28816:
URL: https://github.com/apache/airflow/pull/28816#issuecomment-1376598142
> @vandonr-amz One possible side effect of this change - I think Airflow still can't render dictionaries keys if it templated value: #18938
However, I don't really see why you'd want to template dictionary _keys_. Templating values makes sense, but keys should be pretty much constant
--
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 #28816: introduce a method to convert dictionaries to boto-style key-value lists
Posted by GitBox <gi...@apache.org>.
vandonr-amz commented on PR #28816:
URL: https://github.com/apache/airflow/pull/28816#issuecomment-1377735626
oh so you mean we should always accept either preformated or as a dict. Yeah why not, I can do that.
--
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 #28816: introduce a method to convert dictionaries to boto-style key-value lists
Posted by GitBox <gi...@apache.org>.
vandonr-amz commented on PR #28816:
URL: https://github.com/apache/airflow/pull/28816#issuecomment-1378016820
ok, made it accept a list or dict where meaningful, and added the missing file.
It's also shipping a clandestine commit for now because it's stacked on top of https://github.com/apache/airflow/pull/28837 which has a higher priority.
It should look cleaner once the other commit passes and I can update the base.
--
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 a diff in pull request #28816: introduce a method to convert dictionaries to boto-style key-value lists
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #28816:
URL: https://github.com/apache/airflow/pull/28816#discussion_r1071576124
##########
airflow/providers/amazon/aws/hooks/s3.py:
##########
@@ -42,6 +42,7 @@
from airflow.exceptions import AirflowException
from airflow.providers.amazon.aws.exceptions import S3HookUriParseFailure
from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
+from airflow.providers.amazon.aws.utils.aws_api import format_tags
Review Comment:
Pretty sure Jarek had mentioned not liking stuff in the __init__ files, so I've been trying to avoid it when I can think of a better name. Sometimes it's easier than others. Naming is hard.
In this case I think aws_api.py is a sound option. @eladkal - do you definitely want it changed or were you just making sure I was good with it?
--
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] ashb commented on a diff in pull request #28816: introduce a method to convert dictionaries to boto-style key-value lists
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #28816:
URL: https://github.com/apache/airflow/pull/28816#discussion_r1066882171
##########
docs/apache-airflow-providers-amazon/operators/sagemaker.rst:
##########
@@ -208,6 +208,20 @@ The output is placed in an S3 bucket, and automatically deployed if configured f
:start-after: [START howto_operator_sagemaker_auto_ml]
:end-before: [END howto_operator_sagemaker_auto_ml]
+.. _howto/operator:SageMakerCreateExperimentOperator:
Review Comment:
This all seems to be an unrelated change -- could you pull it out in to a separate PR please?
--
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] ashb commented on a diff in pull request #28816: introduce a method to convert dictionaries to boto-style key-value lists
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #28816:
URL: https://github.com/apache/airflow/pull/28816#discussion_r1071617889
##########
airflow/providers/amazon/aws/hooks/s3.py:
##########
@@ -42,6 +42,7 @@
from airflow.exceptions import AirflowException
from airflow.providers.amazon.aws.exceptions import S3HookUriParseFailure
from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
+from airflow.providers.amazon.aws.utils.aws_api import format_tags
Review Comment:
`airflow/providers/amazon/aws/utils/tags.py`?
--
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] o-nikolas merged pull request #28816: introduce a method to convert dictionaries to boto-style key-value lists
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas merged PR #28816:
URL: https://github.com/apache/airflow/pull/28816
--
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] Taragolis commented on pull request #28816: introduce a method to convert dictionaries to boto-style key-value lists
Posted by GitBox <gi...@apache.org>.
Taragolis commented on PR #28816:
URL: https://github.com/apache/airflow/pull/28816#issuecomment-1376484782
@vandonr-amz One possible side effect of this change - I think Airflow still can't render dictionaries keys if it templated value: https://github.com/apache/airflow/pull/18938
--
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 #28816: introduce a method to convert dictionaries to boto-style key-value lists
Posted by GitBox <gi...@apache.org>.
vandonr-amz commented on PR #28816:
URL: https://github.com/apache/airflow/pull/28816#issuecomment-1376589091
> @vandonr-amz One possible side effect of this change - I think Airflow still can't render dictionaries keys if it templated value: #18938
wow that PR is an adventure... sad to see it doesn't have an happy ending
--
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 #28816: introduce a method to convert dictionaries to boto-style key-value lists
Posted by GitBox <gi...@apache.org>.
vandonr-amz commented on PR #28816:
URL: https://github.com/apache/airflow/pull/28816#issuecomment-1381159841
> * Can this be more-universal across the provider? There are likely other operators that have the same thing, ISTR there are some EC2 operators.
I didn't find other examples of this pattern in the AWS package, RDS, Sagemaker and S3 are the only 3 that popped up, but also it's a bit hard to grep for this.
EC2 apparently only has 2 operators, to start/stop an instance ? And no tag formatting there.
> * Do we show a concrete example of the two styles anywhere in the docs?
> I'm not sure where the best place might be? It almost feels like it should be in a "top level" section like https://airflow.apache.org/docs/apache-airflow-providers-amazon/stable/index.html ? WDYT?
No I don't think we do. It's true that it might be good to have it somewhere, but also, I'd hope for new users to use the dictionary approach, which is a lot more intuitive.
Passing an already formatted list is not marked as so but is almost obsolete, I'm not sure we want to slap something on the front page advertising the complex approach when there is a simple one.
I'd rather copy-paste an example in every docstring where necessary.
--
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] Taragolis commented on pull request #28816: introduce a method to convert dictionaries to boto-style key-value lists
Posted by GitBox <gi...@apache.org>.
Taragolis commented on PR #28816:
URL: https://github.com/apache/airflow/pull/28816#issuecomment-1382212054
> I didn't find other examples of this pattern in the AWS package, RDS, Sagemaker and S3 are the only 3 that popped up, but also it's a bit hard to grep for this.
I know a lot of samples where same approaches uses... but all of them inside of huge object structure.
For example in Batch client:
[SubmitJob](https://docs.aws.amazon.com/batch/latest/APIReference/API_SubmitJob.html)
1. Environment variables in [containerOverrides](https://docs.aws.amazon.com/batch/latest/APIReference/API_SubmitJob.html#Batch-SubmitJob-request-containerOverrides), [EksContainerOverride](https://docs.aws.amazon.com/batch/latest/APIReference/API_EksContainerOverride.html#Batch-Type-EksContainerOverride-env) expected list of dict with "name" and "value" keys
2. [resourceRequirements](https://docs.aws.amazon.com/batch/latest/APIReference/API_ContainerOverrides.html#Batch-Type-ContainerOverrides-resourceRequirements) expected list of dict with "type" and "value" keys
[RegisterJobDefinition](https://docs.aws.amazon.com/batch/latest/APIReference/API_RegisterJobDefinition.html)
1. [environment](https://docs.aws.amazon.com/batch/latest/APIReference/API_ContainerProperties.html#Batch-Type-ContainerProperties-environment) expected list of dict with "name" and "value" keys
2. [secrets](https://docs.aws.amazon.com/batch/latest/APIReference/API_ContainerProperties.html#Batch-Type-ContainerProperties-secrets) expected list of dict with "name" and "valueFrom" keys
So end users might also use this function for produce this objects (I don't think we should help them with this) by just provide dict and appropriate mapping for key_label and value_label.
https://github.com/apache/airflow/blob/e6a037956cb31a883fddc2f9ae9ad5fc0ce541bf/airflow/providers/amazon/aws/utils/aws_api.py#L22
Some almost same function I use in the past in Airflow 1.10.x:
```python
def aws_batch_env(d):
return [{"name": k, "value": v} for k, v in d.items()]
...
batch_env = {
"VAR_1": "{{ ti.xcom_pull(task_ids='foo') }}",
"VAR_2": "{{ ti.xcom_pull(task_ids='bar') }}",
...
"VAR_n": "{{ ti.xcom_pull(task_ids='spam') }}",
}
batch_task = BatchOperator(
...
overrides={
"environment": aws_batch_env(batch_env)
},
)
```
--
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 #28816: introduce a method to convert dictionaries to boto-style key-value lists
Posted by GitBox <gi...@apache.org>.
vandonr-amz commented on code in PR #28816:
URL: https://github.com/apache/airflow/pull/28816#discussion_r1071478212
##########
airflow/providers/amazon/aws/hooks/s3.py:
##########
@@ -42,6 +42,7 @@
from airflow.exceptions import AirflowException
from airflow.providers.amazon.aws.exceptions import S3HookUriParseFailure
from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
+from airflow.providers.amazon.aws.utils.aws_api import format_tags
Review Comment:
I named it boto_ something in my first commit, but then as Taragolis mentioned:
> this is not boto3 behaviour this how what AWS API expected this parameters
I changed it to aws
--
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 a diff in pull request #28816: introduce a method to convert dictionaries to boto-style key-value lists
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #28816:
URL: https://github.com/apache/airflow/pull/28816#discussion_r1071472110
##########
airflow/providers/amazon/aws/hooks/s3.py:
##########
@@ -42,6 +42,7 @@
from airflow.exceptions import AirflowException
from airflow.providers.amazon.aws.exceptions import S3HookUriParseFailure
from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
+from airflow.providers.amazon.aws.utils.aws_api import format_tags
Review Comment:
I'm not sure what else you;'d name the module here that would make more sense? It could be `boto_api` or `boto_utils` maybe but I'm not sure either of those are any better, really?
--
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] ashb commented on a diff in pull request #28816: introduce a method to convert dictionaries to boto-style key-value lists
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #28816:
URL: https://github.com/apache/airflow/pull/28816#discussion_r1066882171
##########
docs/apache-airflow-providers-amazon/operators/sagemaker.rst:
##########
@@ -208,6 +208,20 @@ The output is placed in an S3 bucket, and automatically deployed if configured f
:start-after: [START howto_operator_sagemaker_auto_ml]
:end-before: [END howto_operator_sagemaker_auto_ml]
+.. _howto/operator:SageMakerCreateExperimentOperator:
Review Comment:
This seems to be an unrelated change -- could you pull it out in to a separate PR please?
##########
docs/apache-airflow-providers-amazon/operators/sagemaker.rst:
##########
@@ -208,6 +208,20 @@ The output is placed in an S3 bucket, and automatically deployed if configured f
:start-after: [START howto_operator_sagemaker_auto_ml]
:end-before: [END howto_operator_sagemaker_auto_ml]
+.. _howto/operator:SageMakerCreateExperimentOperator:
Review Comment:
This all seems to be an unrelated change -- could you pull it out in to a separate PR please?
--
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] Taragolis commented on a diff in pull request #28816: introduce a method to convert dictionaries to boto-style key-value lists
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #28816:
URL: https://github.com/apache/airflow/pull/28816#discussion_r1071479962
##########
airflow/providers/amazon/aws/hooks/s3.py:
##########
@@ -42,6 +42,7 @@
from airflow.exceptions import AirflowException
from airflow.providers.amazon.aws.exceptions import S3HookUriParseFailure
from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
+from airflow.providers.amazon.aws.utils.aws_api import format_tags
Review Comment:
Maybe a stupid idea but what if we place it in `airflow/providers/amazon/aws/utils/__init__.py` or it for generic stuff only?
--
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 a diff in pull request #28816: introduce a method to convert dictionaries to boto-style key-value lists
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on code in PR #28816:
URL: https://github.com/apache/airflow/pull/28816#discussion_r1071626275
##########
airflow/providers/amazon/aws/hooks/s3.py:
##########
@@ -42,6 +42,7 @@
from airflow.exceptions import AirflowException
from airflow.providers.amazon.aws.exceptions import S3HookUriParseFailure
from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
+from airflow.providers.amazon.aws.utils.aws_api import format_tags
Review Comment:
I could get behind that.
--
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 a diff in pull request #28816: introduce a method to convert dictionaries to boto-style key-value lists
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #28816:
URL: https://github.com/apache/airflow/pull/28816#discussion_r1070637175
##########
airflow/providers/amazon/aws/hooks/s3.py:
##########
@@ -42,6 +42,7 @@
from airflow.exceptions import AirflowException
from airflow.providers.amazon.aws.exceptions import S3HookUriParseFailure
from airflow.providers.amazon.aws.hooks.base_aws import AwsBaseHook
+from airflow.providers.amazon.aws.utils.aws_api import format_tags
Review Comment:
Why the` aws_` prefix? Lets please double check with @ferruzzi as he lead the effort of making AWS provider comply with AIP-21
--
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