You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "sseung00921 (via GitHub)" <gi...@apache.org> on 2023/11/15 06:45:18 UTC
[PR] Added name field to template_fields in EmrServerlessStartJobOperator [airflow]
sseung00921 opened a new pull request, #35648:
URL: https://github.com/apache/airflow/pull/35648
Added name field to template_fields in EmrServerlessStartJobOperator as requested in issue #35341
At this issue, It is said that [self.name = name or self.config.pop("name", f"emr_serverless_job_airflow_{uuid4()}")](https://github.com/apache/airflow/blob/main/airflow/providers/amazon/aws/operators/emr.py#L1229) code in class constructor makes side effect of removing "name" parameter from "config" dict when applying jinja template in name field.
But, if a value is given to name field, the "self.config.pop..." code dosen't execute. So the side effect dosen't occurs and we can apply Jinja template to name field without the side effect.
Only when name field is given no value, the "self.config.pop..." code execute. And It may be intended flow.
So, I just added this field to template_fields.
@Taragolis
I got kind feedback about this issue from him before. His suggestion is not fully applied in this PR because I think this commit might be enough.
But I might be missing something. If you could give me any insight, I would be really thankful for it.
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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
Re: [PR] Added name field to template_fields in EmrServerlessStartJobOperator [airflow]
Posted by "sseung00921 (via GitHub)" <gi...@apache.org>.
sseung00921 commented on PR #35648:
URL: https://github.com/apache/airflow/pull/35648#issuecomment-1812022808
@Taragolis I will look into it. 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
Re: [PR] Added name field to template_fields in EmrServerlessStartJobOperator [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #35648:
URL: https://github.com/apache/airflow/pull/35648#issuecomment-1812014159
My initial point was, that this operator won't work well with all cases of templated fields around of `name`, so better fix it now and test, rather then keep of "one day in the future"
Example when this operator would fail during initialisation
```python
import pendulum
from airflow.decorators import task
from airflow.models.dag import DAG
from airflow.providers.amazon.aws.operators.emr import EmrServerlessStartJobOperator
SPARK_JOB_DRIVER = {
"sparkSubmit": {
"entryPoint": "spam",
"entryPointArguments": ["s3://spam/egg/output"],
"sparkSubmitParameters": "--conf spark.executor.cores=1 --conf spark.executor.memory=4g\
--conf spark.driver.cores=1 --conf spark.driver.memory=4g --conf spark.executor.instances=1",
}
}
SPARK_CONFIGURATION_OVERRIDES = {
"monitoringConfiguration": {"s3MonitoringConfiguration": {"logUri": "s3://spam/egg/logs"}}
}
with DAG(
"pr_35648",
start_date=pendulum.datetime(2023, 6, 1, tz="UTC"),
schedule=None,
catchup=False,
tags=["pr", "35648"]
):
@task
def emr_config():
return {
"name": "new_application"
}
config = emr_config()
EmrServerlessStartJobOperator(
task_id="create_emr_serverless_task",
application_id="foo",
aws_conn_id=None,
release_label="emr-6.6.0",
job_type="SPARK",
config=config,
configuration_overrides={},
execution_role_arn="foo-bar",
job_driver=SPARK_JOB_DRIVER
)
```
```console
...
self.name = name or self.config.pop("name", f"emr_serverless_job_airflow_{uuid4()}")
AttributeError: 'PlainXComArg' object has no attribute 'pop'
```
I have no objection to add only `name` as templated field and keep it for "better time" in the future. But I think it is a good time to fix this behaviour.
--
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
Re: [PR] Added name field to template_fields in EmrServerlessStartJobOperator [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #35648:
URL: https://github.com/apache/airflow/pull/35648
--
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