Re: [PR] Support multiple executors in chart [airflow]
ihnokim commented on PR #43606: URL: https://github.com/apache/airflow/pull/43606#issuecomment-2811798559 I tried to deploy the chart from this branch with ```executor: "CeleryExecutor,KubernetesExecutor"```, but I still get this error message: ```sh $ helm upgrade --install -n airflow airflow airflow/chart Error: UPGRADE FAILED: failed to create resource: Deployment.apps "airflow-scheduler" is invalid: metadata.labels: Invalid value: "CeleryExecutor,KubernetesExecutor": a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyValue', or 'my_value', or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?') ``` How can I resolve this error? Could you let me know the commits to resolve 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Support multiple executors in chart [airflow]
millin commented on PR #43606: URL: https://github.com/apache/airflow/pull/43606#issuecomment-2703728098 It seems the release of version 1.16.0 has been delayed too long 🥲 -- 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
Re: [PR] Support multiple executors in chart [airflow]
jscheffl commented on PR #43606: URL: https://github.com/apache/airflow/pull/43606#issuecomment-2548826358 > @romsharon98 thanks a lot for your support. One question: is it possible to configure Airflow Helm chart 1.15.0 to use multiple executors? I tried to change the executor via `config:` section in `values.yaml ` like: > > ``` > config: > core: > executor: KubernetesExecutor, CeleryExecutor > ``` > > but during deployment I get error: `helm.go:84: [debug] execution error at (airflow/charts/airflow/templates/NOTES.txt:205:6): Please configure the executor with 'executor', not 'config.core.executor'.` Approach though env variable also doesn't work: > > ``` > env: > - name: AIRFLOW__CORE__EXECUTOR > value: "KubernetesExecutor,CeleryExecutor,LocalExecutor" > ``` > > I am a bit confused. How to change the executor config in airflow.cfg without using approaches like adding `executor: "KubernetesExecutor, CeleryExecutor"` or`config.core.executor: KubernetesExecutor, CeleryExecutor` in values.yaml file. I cannot find some workaround to use multiple executors while deploying Airflow via Airflow Helm Chart 1.15.0 You need to wait for 1.16.0. That makes it "proper". P.S.: We "hacked" it manually (using helmfile and some extra ENV) but if you are not an helm expert I'd rather not propose to patch-around. Chart 1.16 is not too far away. -- 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
Re: [PR] Support multiple executors in chart [airflow]
seniuts-b2 commented on PR #43606: URL: https://github.com/apache/airflow/pull/43606#issuecomment-2548363309 @romsharon98 thanks a lot for your support. One question: is it possible to configure Airflow Helm chart 1.15.0 to use multiple executors? I tried to change the executor via `config:` section in `values.yaml ` like: ``` config: core: executor: KubernetesExecutor, CeleryExecutor ``` but during deployment I get error: `helm.go:84: [debug] execution error at (airflow/charts/airflow/templates/NOTES.txt:205:6): Please configure the executor with `executor`, not `config.core.executor`.` I am a bit confused. How to change the executor config in airflow.cfg without using approaches like adding `executor: "KubernetesExecutor, CeleryExecutor"` or`config.core.executor: KubernetesExecutor, CeleryExecutor`. I cannot find some workaround to use multiple executors while deploying Airflow via Airflow Helm Chart 1.15.0 -- 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
Re: [PR] Support multiple executors in chart [airflow]
romsharon98 commented on PR #43606: URL: https://github.com/apache/airflow/pull/43606#issuecomment-2546198765 This changes didn't release yet in 1.15.0. They should be released on 1.16.0 (you can see the taged milestone in the PR) If you want to use multiple executors like written in the [documentation](https://airflow.apache.org/docs/apache-airflow/stable/core-concepts/executor/index.html#statically-coded-hybrid-executors) -- 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
Re: [PR] Support multiple executors in chart [airflow]
seniuts-b2 commented on PR #43606: URL: https://github.com/apache/airflow/pull/43606#issuecomment-2546133693 @romsharon98 1.15.0 -- 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
Re: [PR] Support multiple executors in chart [airflow]
romsharon98 commented on PR #43606: URL: https://github.com/apache/airflow/pull/43606#issuecomment-2546100740 What chart version are you trying to deploy? -- 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
Re: [PR] Support multiple executors in chart [airflow]
seniuts-b2 commented on PR #43606: URL: https://github.com/apache/airflow/pull/43606#issuecomment-2546011276 @romsharon98 the error looks like: ‘only one the following executors can be used: (here is the list of available executors)’ -- 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
Re: [PR] Support multiple executors in chart [airflow]
romsharon98 commented on PR #43606: URL: https://github.com/apache/airflow/pull/43606#issuecomment-2545928236 You can use it like you wrote, define it like this: ```executor: "CeleryExecutor,KubernetesExecutor"``` What the error you get? -- 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
Re: [PR] Support multiple executors in chart [airflow]
seniuts-b2 commented on PR #43606: URL: https://github.com/apache/airflow/pull/43606#issuecomment-2545888428 @romsharon98 I try to deploy Airflow 2.10.3 to Azure k8s via helm chart and I want to use multiple executors (Celery and Kubernetes) in my DAGs. As I understand in my values.yaml in section _executor:_ I need to provide list of executors that I want. I cannot understand hot to do 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Support multiple executors in chart [airflow]
romsharon98 commented on PR #43606: URL: https://github.com/apache/airflow/pull/43606#issuecomment-2545873578 > @romsharon98 > > Could you please explain: How I can pass multiple executors to custom values.yaml file? > > These don't work for me: > > `executor: "KubernetesExecutor, CeleryExecutor"` > > `executor: KubernetesExecutor, CeleryExecutor` Can you explain what exactly you get when you try this? -- 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
Re: [PR] Support multiple executors in chart [airflow]
seniuts-b2 commented on PR #43606: URL: https://github.com/apache/airflow/pull/43606#issuecomment-2545866000 Could some one please explain: How I can pass multiple executors to custom values.yaml file? These don't work for me: `executor: "KubernetesExecutor, CeleryExecutor"` `executor: KubernetesExecutor, CeleryExecutor` -- 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
Re: [PR] Support multiple executors in chart [airflow]
romsharon98 merged PR #43606: URL: https://github.com/apache/airflow/pull/43606 -- 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
Re: [PR] Support multiple executors in chart [airflow]
jx2lee commented on code in PR #43606:
URL: https://github.com/apache/airflow/pull/43606#discussion_r1849694651
##
helm_tests/other/test_keda.py:
##
@@ -121,6 +121,8 @@ def test_keda_concurrency(self, executor, concurrency):
("CeleryExecutor", "my_queue", False),
("CeleryKubernetesExecutor", None, True),
("CeleryKubernetesExecutor", "my_queue", True),
+("CeleryExecutor,KubernetesExecutor", "None", False),
Review Comment:
fixed 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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Support multiple executors in chart [airflow]
romsharon98 commented on code in PR #43606:
URL: https://github.com/apache/airflow/pull/43606#discussion_r1849667631
##
helm_tests/other/test_keda.py:
##
@@ -121,6 +121,8 @@ def test_keda_concurrency(self, executor, concurrency):
("CeleryExecutor", "my_queue", False),
("CeleryKubernetesExecutor", None, True),
("CeleryKubernetesExecutor", "my_queue", True),
+("CeleryExecutor,KubernetesExecutor", "None", False),
Review Comment:
```suggestion
("CeleryExecutor,KubernetesExecutor", None, True),
```
--
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
Re: [PR] Support multiple executors in chart [airflow]
jx2lee commented on code in PR #43606: URL: https://github.com/apache/airflow/pull/43606#discussion_r1848590111 ## chart/values.schema.json: ## @@ -687,15 +687,7 @@ "type": "string", "x-docsSection": "Common", "default": "CeleryExecutor", -"enum": [ -"LocalExecutor", -"LocalKubernetesExecutor", -"CeleryExecutor", -"KubernetesExecutor", -"CeleryKubernetesExecutor", - "airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor", -"airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor" -] +"pattern": "^(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor)(,(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor))*$" Review Comment: ++ modifed helm test about keda -- 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
Re: [PR] Support multiple executors in chart [airflow]
jx2lee commented on code in PR #43606: URL: https://github.com/apache/airflow/pull/43606#discussion_r1848590111 ## chart/values.schema.json: ## @@ -687,15 +687,7 @@ "type": "string", "x-docsSection": "Common", "default": "CeleryExecutor", -"enum": [ -"LocalExecutor", -"LocalKubernetesExecutor", -"CeleryExecutor", -"KubernetesExecutor", -"CeleryKubernetesExecutor", - "airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor", -"airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor" -] +"pattern": "^(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor)(,(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor))*$" Review Comment: ++ modified helm test about keda -- 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
Re: [PR] Support multiple executors in chart [airflow]
jx2lee commented on code in PR #43606: URL: https://github.com/apache/airflow/pull/43606#discussion_r1848098557 ## chart/values.schema.json: ## @@ -687,15 +687,7 @@ "type": "string", "x-docsSection": "Common", "default": "CeleryExecutor", -"enum": [ -"LocalExecutor", -"LocalKubernetesExecutor", -"CeleryExecutor", -"KubernetesExecutor", -"CeleryKubernetesExecutor", - "airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor", -"airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor" -] +"pattern": "^(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor)(,(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor))*$" Review Comment: @romsharon98 in [this commit](https://github.com/apache/airflow/commit/3136f22bf7377ed177d7dcca795339c674459913), need to prevent count on k8s queue. come to think, you are right! I'll make change for now. -- 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
Re: [PR] Support multiple executors in chart [airflow]
romsharon98 commented on code in PR #43606: URL: https://github.com/apache/airflow/pull/43606#discussion_r1847708118 ## chart/values.schema.json: ## @@ -687,15 +687,7 @@ "type": "string", "x-docsSection": "Common", "default": "CeleryExecutor", -"enum": [ -"LocalExecutor", -"LocalKubernetesExecutor", -"CeleryExecutor", -"KubernetesExecutor", -"CeleryKubernetesExecutor", - "airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor", -"airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor" -] +"pattern": "^(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor)(,(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor))*$" Review Comment: After this change, we can set executor="CeleryExecutor,KubernetesExecutor". Therefore, if there are tasks running on the KubernetesExecutor, we want to filter out tasks meant for Celery by using a queue filter to decide whether to increase the number of Celery workers. -- 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
Re: [PR] Support multiple executors in chart [airflow]
romsharon98 commented on code in PR #43606: URL: https://github.com/apache/airflow/pull/43606#discussion_r1847708118 ## chart/values.schema.json: ## @@ -687,15 +687,7 @@ "type": "string", "x-docsSection": "Common", "default": "CeleryExecutor", -"enum": [ -"LocalExecutor", -"LocalKubernetesExecutor", -"CeleryExecutor", -"KubernetesExecutor", -"CeleryKubernetesExecutor", - "airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor", -"airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor" -] +"pattern": "^(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor)(,(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor))*$" Review Comment: After this change, we can set executor="CeleryExecutor,KubernetesExecutor". Therefore, if there are tasks running on the KubernetesExecutor, we want to filter out tasks meant for Celery by using a queue filter to decide whether to increase the number of Celery workers. Maybe @hussein-awala can help us to decide -- 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
Re: [PR] Support multiple executors in chart [airflow]
jx2lee commented on code in PR #43606: URL: https://github.com/apache/airflow/pull/43606#discussion_r1847482327 ## chart/values.schema.json: ## @@ -687,15 +687,7 @@ "type": "string", "x-docsSection": "Common", "default": "CeleryExecutor", -"enum": [ -"LocalExecutor", -"LocalKubernetesExecutor", -"CeleryExecutor", -"KubernetesExecutor", -"CeleryKubernetesExecutor", - "airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor", -"airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor" -] +"pattern": "^(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor)(,(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor))*$" Review Comment: If the `workers.keda.query` option is enabled, is it also used when using the `KubernetesExecutor`? I haven't tested it with KEDA yet. (also CeleryKubernetesExecutor..) -- 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
Re: [PR] Support multiple executors in chart [airflow]
jx2lee commented on code in PR #43606: URL: https://github.com/apache/airflow/pull/43606#discussion_r1847482327 ## chart/values.schema.json: ## @@ -687,15 +687,7 @@ "type": "string", "x-docsSection": "Common", "default": "CeleryExecutor", -"enum": [ -"LocalExecutor", -"LocalKubernetesExecutor", -"CeleryExecutor", -"KubernetesExecutor", -"CeleryKubernetesExecutor", - "airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor", -"airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor" -] +"pattern": "^(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor)(,(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor))*$" Review Comment: If the `workers.keda.query` option is enabled, is it also used when using the `KubernetesExecutor`? I haven't tried it with KEDA yet. (also CeleryKubernetesExecutor..) Im not sure if it's necessary to include `KubernetesExecutor`. Could you explain why add to KubernetesExecutor? -- 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
Re: [PR] Support multiple executors in chart [airflow]
jx2lee commented on code in PR #43606: URL: https://github.com/apache/airflow/pull/43606#discussion_r1847482327 ## chart/values.schema.json: ## @@ -687,15 +687,7 @@ "type": "string", "x-docsSection": "Common", "default": "CeleryExecutor", -"enum": [ -"LocalExecutor", -"LocalKubernetesExecutor", -"CeleryExecutor", -"KubernetesExecutor", -"CeleryKubernetesExecutor", - "airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor", -"airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor" -] +"pattern": "^(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor)(,(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor))*$" Review Comment: If the `workers.keda.query` option is enabled, is it also used when using the `KubernetesExecutor`? I haven't tested it with KEDA yet. (also CeleryKubernetesExecutor..) Im not sure if it's necessary to include `KubernetesExecutor`. Could you explain why add to KubernetesExecutor? -- 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
Re: [PR] Support multiple executors in chart [airflow]
jx2lee commented on code in PR #43606: URL: https://github.com/apache/airflow/pull/43606#discussion_r1847482327 ## chart/values.schema.json: ## @@ -687,15 +687,7 @@ "type": "string", "x-docsSection": "Common", "default": "CeleryExecutor", -"enum": [ -"LocalExecutor", -"LocalKubernetesExecutor", -"CeleryExecutor", -"KubernetesExecutor", -"CeleryKubernetesExecutor", - "airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor", -"airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor" -] +"pattern": "^(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor)(,(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor))*$" Review Comment: If the `workers.keda.query` option is enabled, is it also used when using the `KubernetesExecutor`? I haven't tested it with KEDA yet. (also CeleryKubernetesExecutor..) Im not sure if it's necessary to include `KubernetesExecutor`. -- 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
Re: [PR] Support multiple executors in chart [airflow]
romsharon98 commented on code in PR #43606: URL: https://github.com/apache/airflow/pull/43606#discussion_r1846428097 ## chart/values.schema.json: ## @@ -687,15 +687,7 @@ "type": "string", "x-docsSection": "Common", "default": "CeleryExecutor", -"enum": [ -"LocalExecutor", -"LocalKubernetesExecutor", -"CeleryExecutor", -"KubernetesExecutor", -"CeleryKubernetesExecutor", - "airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor", -"airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor" -] +"pattern": "^(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor)(,(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor))*$" Review Comment: I think change affect this: https://github.com/apache/airflow/blob/main/chart/values.yaml#L658 we need add if executor contains KubernetesExecutor. -- 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
Re: [PR] Support multiple executors in chart [airflow]
romsharon98 commented on code in PR #43606: URL: https://github.com/apache/airflow/pull/43606#discussion_r1846717944 ## chart/values.schema.json: ## @@ -687,15 +687,7 @@ "type": "string", "x-docsSection": "Common", "default": "CeleryExecutor", -"enum": [ -"LocalExecutor", -"LocalKubernetesExecutor", -"CeleryExecutor", -"KubernetesExecutor", -"CeleryKubernetesExecutor", - "airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor", -"airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor" -] +"pattern": "^(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor)(,(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor))*$" Review Comment: Shouldn't we check if executor contains KubernetesExecutor too? -- 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
Re: [PR] Support multiple executors in chart [airflow]
romsharon98 commented on code in PR #43606:
URL: https://github.com/apache/airflow/pull/43606#discussion_r1846436270
##
helm_tests/airflow_aux/test_basic_helm_chart.py:
##
@@ -549,19 +549,20 @@ def test_supported_executor(self, executor):
)
def test_unsupported_executor(self):
-with pytest.raises(CalledProcessError) as ex_ctx:
+with pytest.raises(CalledProcessError):
render_chart(
"test-basic",
{
"executor": "SequentialExecutor",
},
)
-assert (
-'executor must be one of the following: "LocalExecutor", '
-'"LocalKubernetesExecutor", "CeleryExecutor", '
-'"KubernetesExecutor", "CeleryKubernetesExecutor", '
-'"airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor", '
-'"airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor"' in
ex_ctx.value.stderr.decode()
+
+def test_support_multiple_executors(self):
Review Comment:
I think better combine those tests: test_support_multiple_executors,
test_supported_executor
--
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
Re: [PR] Support multiple executors in chart [airflow]
romsharon98 commented on code in PR #43606: URL: https://github.com/apache/airflow/pull/43606#discussion_r1846428097 ## chart/values.schema.json: ## @@ -687,15 +687,7 @@ "type": "string", "x-docsSection": "Common", "default": "CeleryExecutor", -"enum": [ -"LocalExecutor", -"LocalKubernetesExecutor", -"CeleryExecutor", -"KubernetesExecutor", -"CeleryKubernetesExecutor", - "airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor", -"airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor" -] +"pattern": "^(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor)(,(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor))*$" Review Comment: I think change affect this: https://github.com/apache/airflow/blob/main/chart/values.yaml#L658 we need add here if executor contains KubernetesExecutor. -- 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
Re: [PR] Support multiple executors in chart [airflow]
jx2lee commented on code in PR #43606: URL: https://github.com/apache/airflow/pull/43606#discussion_r1846553872 ## chart/values.schema.json: ## @@ -687,15 +687,7 @@ "type": "string", "x-docsSection": "Common", "default": "CeleryExecutor", -"enum": [ -"LocalExecutor", -"LocalKubernetesExecutor", -"CeleryExecutor", -"KubernetesExecutor", -"CeleryKubernetesExecutor", - "airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor", -"airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor" -] +"pattern": "^(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor)(,(LocalExecutor|LocalKubernetesExecutor|CeleryExecutor|KubernetesExecutor|CeleryKubernetesExecutor|airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor|airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor))*$" Review Comment: Thanks! fixed 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Support multiple executors in chart [airflow]
jx2lee commented on code in PR #43606:
URL: https://github.com/apache/airflow/pull/43606#discussion_r1846501134
##
helm_tests/airflow_aux/test_basic_helm_chart.py:
##
@@ -549,19 +549,20 @@ def test_supported_executor(self, executor):
)
def test_unsupported_executor(self):
-with pytest.raises(CalledProcessError) as ex_ctx:
+with pytest.raises(CalledProcessError):
render_chart(
"test-basic",
{
"executor": "SequentialExecutor",
},
)
-assert (
-'executor must be one of the following: "LocalExecutor", '
-'"LocalKubernetesExecutor", "CeleryExecutor", '
-'"KubernetesExecutor", "CeleryKubernetesExecutor", '
-'"airflow.providers.amazon.aws.executors.batch.AwsBatchExecutor", '
-'"airflow.providers.amazon.aws.executors.ecs.AwsEcsExecutor"' in
ex_ctx.value.stderr.decode()
+
+def test_support_multiple_executors(self):
Review Comment:
Thanks! I combined tests.
--
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
Re: [PR] Support multiple executors in chart [airflow]
joshua-yeung-mox commented on PR #43606: URL: https://github.com/apache/airflow/pull/43606#issuecomment-2463728827 We are waiting for this feature. Hope it can be released soon. -- 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
Re: [PR] Support multiple executors in chart [airflow]
jscheffl commented on PR #43606: URL: https://github.com/apache/airflow/pull/43606#issuecomment-2453109835 Relates to #41524 and #41797 -- 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
