You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Usiel (via GitHub)" <gi...@apache.org> on 2023/07/20 07:22:15 UTC
[GitHub] [airflow] Usiel opened a new pull request, #32709: Enable pools to consider deferred tasks
Usiel opened a new pull request, #32709:
URL: https://github.com/apache/airflow/pull/32709
<!--
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:
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
closes: https://github.com/apache/airflow/discussions/21243
This is my attempt at solving the issue raised in the linked discussion. With this change we would introduce a new flag on `Pool`, that allows an admin to decide whether to include tasks in deferred state in the pool open slot calculation or not. By default we would not consider them, which mirrors the current behavior.
As mentioned by various people on the discussion, having this as an option can be quite useful, especially in cases where the system that is "shielded" by the pool is not completely in the control of the developer. This allows us to (mis)use pools as a rate-limiting device even when using the new deferrable operators.
--
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] boring-cyborg[bot] commented on pull request #32709: Enable pools to consider deferred tasks
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #32709:
URL: https://github.com/apache/airflow/pull/32709#issuecomment-1643411156
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (ruff, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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] Usiel commented on a diff in pull request #32709: Enable pools to consider deferred tasks
Posted by "Usiel (via GitHub)" <gi...@apache.org>.
Usiel commented on code in PR #32709:
URL: https://github.com/apache/airflow/pull/32709#discussion_r1282817431
##########
airflow/api/client/api_client.py:
##########
@@ -60,12 +60,13 @@ def get_pools(self):
"""Get all pools."""
raise NotImplementedError()
- def create_pool(self, name, slots, description):
+ def create_pool(self, name, slots, description, include_deferred):
Review Comment:
Internally we cover all our bases upstream of the client, unless I'm missing something. The `pool_command` module is the only consumer and in both usages we handle a missing `include_deferred`:
- When importing a pool from JSON we handle the missing flag and fall back to `False` (https://github.com/apache/airflow/pull/32709/files#diff-b16c716393f690332e062e25defef0da5ac27e282553040b4b4dee223f1d169fR131)
- When upserting a pool (coming from `airflow pools set ...`) we have an _optional_ flag for the CLI command (so the default is `False`) (https://github.com/apache/airflow/pull/32709/files#diff-6e938311edf46fbd7341cc3f151407931322af0121acbfca9885b9f3aa0a8565R516-R518)
Is `get_current_api_client()` (`airflow.api.client`) something that is meant to be used to interact with Airflow? If yes, then yes, I agree we should change the signature as you said. I couldn't find much on this topic, [Public Interface of Airflow docs](https://airflow.apache.org/docs/apache-airflow/stable/public-airflow-interface.html) only makes mention of the stable REST API and CLI.
--
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 pull request #32709: Enable pools to consider deferred tasks
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #32709:
URL: https://github.com/apache/airflow/pull/32709#issuecomment-1651099280
Just set the default to false, don’t bother much about the deprecated API.
--
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 pull request #32709: Enable pools to consider deferred tasks
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #32709:
URL: https://github.com/apache/airflow/pull/32709#issuecomment-1663431164
Can you rebase and resolve conflicts?
--
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 pull request #32709: Enable pools to consider deferred tasks
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32709:
URL: https://github.com/apache/airflow/pull/32709#issuecomment-1671178859
Let's just run tests again after rebase and 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] Usiel commented on pull request #32709: Enable pools to consider deferred tasks
Posted by "Usiel (via GitHub)" <gi...@apache.org>.
Usiel commented on PR #32709:
URL: https://github.com/apache/airflow/pull/32709#issuecomment-1652828733
> I am not in favor of making `include_deferred` a `required_field`, I think it can just default to `False` (as you did) and keep the code as it is. (no need for custom `include_deferred` handling and deprecation such as `_handle_missing_include_deferred`),
I'm happy to rework the PR with a nullable field (with default). I agree, it does circumvent a lot of the workarounds and extra steps, especially in the last few commits in regards to backwards compatibility. Before I get started I'll let @uranusjr chime in, if there is any stronger argument to be made for keeping it as non-nullable.
Thanks everyone for the reviews so far and guiding me here for my first PR on this repo.
Btw, the test failure (`test_databricks.py::TestDatabricksExecutionTrigger::test_sleep_between_retries`) seems unrelated and I cannot reproduce a failure.
--
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 pull request #32709: Enable pools to consider deferred tasks
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32709:
URL: https://github.com/apache/airflow/pull/32709#issuecomment-1671170908
Yeah. My bad. In fact the client is **still** used internally by pool command it seems. So yeah. My change was too much - will remove it and only remove stuff from experimental client
--
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] pierrejeambrun commented on pull request #32709: Enable pools to consider deferred tasks
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #32709:
URL: https://github.com/apache/airflow/pull/32709#issuecomment-1651490289
I am not in favor of making `include_deferred` a `required_field`, I think it can just default to `False` (as you did) and keep the code as it is. (no need for custom `include_deferred` handling and deprecation such as `_handle_missing_include_deferred`),
--
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] Usiel commented on a diff in pull request #32709: Enable pools to consider deferred tasks
Posted by "Usiel (via GitHub)" <gi...@apache.org>.
Usiel commented on code in PR #32709:
URL: https://github.com/apache/airflow/pull/32709#discussion_r1271736556
##########
airflow/migrations/versions/0128_2_7_0_add_include_deferred_column_to_pool.py:
##########
@@ -0,0 +1,48 @@
+#
+# 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.
+
+"""add include_deferred column to pool
+
+Revision ID: 405de8318b3a
+Revises: 788397e78828
+Create Date: 2023-07-20 04:22:21.007342
+
+"""
+
+import sqlalchemy as sa
+from alembic import op
+
+
+# revision identifiers, used by Alembic.
+revision = "405de8318b3a"
+down_revision = "788397e78828"
+branch_labels = None
+depends_on = None
+airflow_version = "2.7.0"
+
+
+def upgrade():
+ """Apply add include_deferred column to pool"""
+ with op.batch_alter_table("slot_pool") as batch_op:
+ batch_op.add_column(sa.Column("include_deferred", sa.Boolean, default=False))
Review Comment:
Fixed: Added the then required `server_default` and also explicitly set the same arg on the `Pool` class.
--
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] Usiel commented on a diff in pull request #32709: Enable pools to consider deferred tasks
Posted by "Usiel (via GitHub)" <gi...@apache.org>.
Usiel commented on code in PR #32709:
URL: https://github.com/apache/airflow/pull/32709#discussion_r1281588863
##########
tests/cli/commands/test_pool_command.py:
##########
@@ -88,19 +100,34 @@ def test_pool_import_invalid_json(self):
pool_command.pool_import(self.parser.parse_args(["pools", "import", "pools_import_invalid.json"]))
def test_pool_import_invalid_pools(self):
- pool_config_input = {"foo": {"description": "foo_test"}}
+ pool_config_input = {"foo": {"description": "foo_test", "include_deferred": False}}
with open("pools_import_invalid.json", mode="w") as file:
json.dump(pool_config_input, file)
with pytest.raises(SystemExit):
pool_command.pool_import(self.parser.parse_args(["pools", "import", "pools_import_invalid.json"]))
+ def test_pool_import_backwards_compatibility(self):
+ try:
+ pool_config_input = {
+ # JSON before version 2.7.0 does not contain `include_deferred`
+ "foo": {"description": "foo_test", "slots": 1},
+ }
+ with open("pools_import.json", mode="w") as file:
+ json.dump(pool_config_input, file)
+
+ pool_command.pool_import(self.parser.parse_args(["pools", "import", "pools_import.json"]))
+
+ assert self.session.query(Pool).filter(Pool.pool == "foo").first().include_deferred is False
+ finally:
+ os.remove("pools_import.json")
Review Comment:
Oh nice! Fixed up the remaining tests as well.
`tmp_path` seems to be the new thing replacing tmpdir ([ref](https://docs.pytest.org/en/7.1.x/how-to/tmp_path.html#the-tmpdir-and-tmpdir-factory-fixtures)).
--
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 #32709: Enable pools to consider deferred tasks
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #32709:
URL: https://github.com/apache/airflow/pull/32709#discussion_r1271686115
##########
airflow/migrations/versions/0128_2_7_0_add_include_deferred_column_to_pool.py:
##########
@@ -0,0 +1,48 @@
+#
+# 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.
+
+"""add include_deferred column to pool
+
+Revision ID: 405de8318b3a
+Revises: 788397e78828
+Create Date: 2023-07-20 04:22:21.007342
+
+"""
+
+import sqlalchemy as sa
+from alembic import op
+
+
+# revision identifiers, used by Alembic.
+revision = "405de8318b3a"
+down_revision = "788397e78828"
+branch_labels = None
+depends_on = None
+airflow_version = "2.7.0"
+
+
+def upgrade():
+ """Apply add include_deferred column to pool"""
+ with op.batch_alter_table("slot_pool") as batch_op:
+ batch_op.add_column(sa.Column("include_deferred", sa.Boolean, default=False))
Review Comment:
This should be `nullable=False`
--
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 pull request #32709: Enable pools to consider deferred tasks
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32709:
URL: https://github.com/apache/airflow/pull/32709#issuecomment-1671177749
Re-pushed original change - the back compatibiliyt is not needed there and the change was good as it was! I completely forgot that we used to use the "CLI API" before - but it's really not part of the public API since 2.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-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 #32709: Enable pools to consider deferred tasks
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #32709:
URL: https://github.com/apache/airflow/pull/32709#discussion_r1272919829
##########
airflow/models/pool.py:
##########
@@ -161,21 +163,26 @@ def slots_stats(
from airflow.models.taskinstance import TaskInstance # Avoid circular import
pools: dict[str, PoolStats] = {}
+ pool_includes_deferred: dict[str, bool] = {}
- query = select(Pool.pool, Pool.slots)
+ query = select(Pool.pool, Pool.slots, Pool.include_deferred)
if lock_rows:
query = with_row_locks(query, session=session, **nowait(session))
pool_rows = session.execute(query)
- for (pool_name, total_slots) in pool_rows:
+ for (pool_name, total_slots, includes_deferred) in pool_rows:
Review Comment:
Please unify whether you want to use _include_ or _includes_.
##########
airflow/models/pool.py:
##########
@@ -224,15 +236,25 @@ def occupied_slots(self, session: Session = NEW_SESSION) -> int:
"""
from airflow.models.taskinstance import TaskInstance # Avoid circular import
+ occupied_states = Pool.get_occupied_states(self.include_deferred)
+
return int(
session.scalar(
select(func.sum(TaskInstance.pool_slots))
.filter(TaskInstance.pool == self.pool)
- .filter(TaskInstance.state.in_(EXECUTION_STATES))
+ .filter(TaskInstance.state.in_(occupied_states))
)
or 0
)
+ @staticmethod
+ def get_occupied_states(include_deferred):
+ if include_deferred:
+ return EXECUTION_STATES | {
+ TaskInstanceState.DEFERRED,
+ }
+ return EXECUTION_STATES
Review Comment:
Why is this not an instance method so we can simply do `pool.get_occupied_states()` without arguments?
##########
airflow/migrations/versions/0128_2_7_0_add_include_deferred_column_to_pool.py:
##########
@@ -0,0 +1,52 @@
+#
+# 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.
+
+"""add include_deferred column to pool
+
+Revision ID: 405de8318b3a
+Revises: 788397e78828
+Create Date: 2023-07-20 04:22:21.007342
+
+"""
+
+import sqlalchemy as sa
+from alembic import op
+
+
+# revision identifiers, used by Alembic.
+revision = "405de8318b3a"
+down_revision = "788397e78828"
+branch_labels = None
+depends_on = None
+airflow_version = "2.7.0"
+
+
+def upgrade():
+ """Apply add include_deferred column to pool"""
+ with op.batch_alter_table("slot_pool") as batch_op:
+ batch_op.add_column(
+ sa.Column(
+ "include_deferred", sa.Boolean, default=False, nullable=False, server_default=sa.false()
Review Comment:
Do we still need `default`?
##########
airflow/models/pool.py:
##########
@@ -290,6 +312,25 @@ def scheduled_slots(self, session: Session = NEW_SESSION) -> int:
or 0
)
+ @provide_session
+ def deferred_slots(self, session: Session = NEW_SESSION) -> int:
+ """
+ Get the number of slots deferred at the moment.
+
+ :param session: SQLAlchemy ORM Session
+ :return: the number of deferred slots
+ """
+ from airflow.models.taskinstance import TaskInstance # Avoid circular import
+
+ return int(
+ session.scalar(
+ select(func.sum(TaskInstance.pool_slots))
+ .filter(TaskInstance.pool == self.pool)
+ .filter(TaskInstance.state == TaskInstanceState.DEFERRED)
Review Comment:
```suggestion
.where(TaskInstance.pool == self.pool, TaskInstance.state == TaskInstanceState.DEFERRED)
```
--
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] Usiel commented on a diff in pull request #32709: Enable pools to consider deferred tasks
Posted by "Usiel (via GitHub)" <gi...@apache.org>.
Usiel commented on code in PR #32709:
URL: https://github.com/apache/airflow/pull/32709#discussion_r1273029509
##########
airflow/models/pool.py:
##########
@@ -224,15 +236,25 @@ def occupied_slots(self, session: Session = NEW_SESSION) -> int:
"""
from airflow.models.taskinstance import TaskInstance # Avoid circular import
+ occupied_states = Pool.get_occupied_states(self.include_deferred)
+
return int(
session.scalar(
select(func.sum(TaskInstance.pool_slots))
.filter(TaskInstance.pool == self.pool)
- .filter(TaskInstance.state.in_(EXECUTION_STATES))
+ .filter(TaskInstance.state.in_(occupied_states))
)
or 0
)
+ @staticmethod
+ def get_occupied_states(include_deferred):
+ if include_deferred:
+ return EXECUTION_STATES | {
+ TaskInstanceState.DEFERRED,
+ }
+ return EXECUTION_STATES
Review Comment:
Right, that's fixed. Static method was a remnant of a different approach.
##########
airflow/models/pool.py:
##########
@@ -161,21 +163,26 @@ def slots_stats(
from airflow.models.taskinstance import TaskInstance # Avoid circular import
pools: dict[str, PoolStats] = {}
+ pool_includes_deferred: dict[str, bool] = {}
- query = select(Pool.pool, Pool.slots)
+ query = select(Pool.pool, Pool.slots, Pool.include_deferred)
if lock_rows:
query = with_row_locks(query, session=session, **nowait(session))
pool_rows = session.execute(query)
- for (pool_name, total_slots) in pool_rows:
+ for (pool_name, total_slots, includes_deferred) in pool_rows:
Review Comment:
Done
--
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 a diff in pull request #32709: Enable pools to consider deferred tasks
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32709:
URL: https://github.com/apache/airflow/pull/32709#discussion_r1288271088
##########
airflow/api/client/api_client.py:
##########
@@ -60,12 +60,13 @@ def get_pools(self):
"""Get all pools."""
raise NotImplementedError()
- def create_pool(self, name, slots, description):
+ def create_pool(self, name, slots, description, include_deferred):
Review Comment:
Can you remove those changes from "airflow/api/client" packages @Usiel please? I am eyeing to add that change to 2.7.0 if possible, so if there is a way to do it quickly that would be fantastic.
--
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] pierrejeambrun commented on a diff in pull request #32709: Enable pools to consider deferred tasks
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #32709:
URL: https://github.com/apache/airflow/pull/32709#discussion_r1281557441
##########
tests/cli/commands/test_pool_command.py:
##########
@@ -88,19 +100,34 @@ def test_pool_import_invalid_json(self):
pool_command.pool_import(self.parser.parse_args(["pools", "import", "pools_import_invalid.json"]))
def test_pool_import_invalid_pools(self):
- pool_config_input = {"foo": {"description": "foo_test"}}
+ pool_config_input = {"foo": {"description": "foo_test", "include_deferred": False}}
with open("pools_import_invalid.json", mode="w") as file:
json.dump(pool_config_input, file)
with pytest.raises(SystemExit):
pool_command.pool_import(self.parser.parse_args(["pools", "import", "pools_import_invalid.json"]))
+ def test_pool_import_backwards_compatibility(self):
+ try:
+ pool_config_input = {
+ # JSON before version 2.7.0 does not contain `include_deferred`
+ "foo": {"description": "foo_test", "slots": 1},
+ }
+ with open("pools_import.json", mode="w") as file:
+ json.dump(pool_config_input, file)
+
+ pool_command.pool_import(self.parser.parse_args(["pools", "import", "pools_import.json"]))
+
+ assert self.session.query(Pool).filter(Pool.pool == "foo").first().include_deferred is False
+ finally:
+ os.remove("pools_import.json")
Review Comment:
You can use the tmpdir fixture provided by pytest to not have to handle manually this tmp file lifecycle
##########
airflow/api_connexion/endpoints/pool_endpoint.py:
##########
@@ -155,3 +156,13 @@ def post_pool(*, session: Session = NEW_SESSION) -> APIResponse:
return pool_schema.dump(pool)
except IntegrityError:
raise AlreadyExists(detail=f"Pool: {post_body['pool']} already exists")
+
+
+def _handle_missing_include_deferred(fields_diff: set) -> None:
Review Comment:
This should be removed
--
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] Usiel commented on pull request #32709: Enable pools to consider deferred tasks
Posted by "Usiel (via GitHub)" <gi...@apache.org>.
Usiel commented on PR #32709:
URL: https://github.com/apache/airflow/pull/32709#issuecomment-1661713508
Rereading I finally get the original intent (I think) :) Here's that version again, with an additional minor improvement for import/export (import backwards compatible).
--
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] RNHTTR commented on a diff in pull request #32709: Enable pools to consider deferred tasks
Posted by "RNHTTR (via GitHub)" <gi...@apache.org>.
RNHTTR commented on code in PR #32709:
URL: https://github.com/apache/airflow/pull/32709#discussion_r1272305349
##########
docs/apache-airflow/authoring-and-scheduling/deferring.rst:
##########
@@ -20,7 +20,7 @@ Deferrable Operators & Triggers
Standard :doc:`Operators </core-concepts/operators>` and :doc:`Sensors <../core-concepts/sensors>` take up a full *worker slot* for the entire time they are running, even if they are idle; for example, if you only have 100 worker slots available to run Tasks, and you have 100 DAGs waiting on a Sensor that's currently running but idle, then you *cannot run anything else* - even though your entire Airflow cluster is essentially idle. ``reschedule`` mode for Sensors solves some of this, allowing Sensors to only run at fixed intervals, but it is inflexible and only allows using time as the reason to resume, not anything else.
-This is where *Deferrable Operators* come in. A deferrable operator is one that is written with the ability to suspend itself and free up the worker when it knows it has to wait, and hand off the job of resuming it to something called a *Trigger*. As a result, while it is suspended (deferred), it is not taking up a worker slot and your cluster will have a lot less resources wasted on idle Operators or Sensors.
+This is where *Deferrable Operators* come in. A deferrable operator is one that is written with the ability to suspend itself and free up the worker when it knows it has to wait, and hand off the job of resuming it to something called a *Trigger*. As a result, while it is suspended (deferred), it is not taking up a worker slot and your cluster will have a lot less resources wasted on idle Operators or Sensors. Note that by default a deferred tasks will not use up a pool slot, if you would like them to, you can change this by editing the pool in question.
Review Comment:
Typo in the following sentence ("task" should be singular)
> Note that by default a deferred task~s~ will not use up a pool slot, if you would like them to, you can change this by editing the pool in question.
--
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] Usiel commented on pull request #32709: Enable pools to consider deferred tasks
Posted by "Usiel (via GitHub)" <gi...@apache.org>.
Usiel commented on PR #32709:
URL: https://github.com/apache/airflow/pull/32709#issuecomment-1649170249
> Making it nullable would cause the unchecked case to return NULL instead of FALSE, which is not a good idea. NULL in SQL is just bad. I’m pretty sure there’s a way to make a checkbox return true/false instead.
OK, managed to get it working by overriding the form field. Using an additional validator in `validators_columns` was not sufficient, as the `DataRequired` (due to `nullable=False`) is still appended and has some effects for the form template.
https://github.com/apache/airflow/pull/32709/files#diff-917b70d2661fae8322c538a4f60b9d4ba755556036fe0c527eb0411a6ac60f2eR5051-R5056
--
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] Usiel commented on pull request #32709: Enable pools to consider deferred tasks
Posted by "Usiel (via GitHub)" <gi...@apache.org>.
Usiel commented on PR #32709:
URL: https://github.com/apache/airflow/pull/32709#issuecomment-1648930695
> I wonder if we could add some help text in the pool form to explain what the flag means, it’s not as straightfoward as other fields. I’m not sure how to do that though, probably via some WTForm trick.
Looks like Flask AppBuilder [has a feature](https://flask-appbuilder.readthedocs.io/en/latest/api.html#flask_appbuilder.baseviews.BaseCRUDView.description_columns) for that. I'm open for suggestions if someone has a better help text:
(will push this shortly after addressing the other comments)
--
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] Usiel commented on pull request #32709: Enable pools to consider deferred tasks
Posted by "Usiel (via GitHub)" <gi...@apache.org>.
Usiel commented on PR #32709:
URL: https://github.com/apache/airflow/pull/32709#issuecomment-1661689399
Then I don't quite understand the comment by @pierrejeambrun. Is the issue simply with the deprecation logic (i.e. at some point making the field required)?
(I'll push the original branch again in a bit)
--
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 #32709: Enable pools to consider deferred tasks
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #32709:
URL: https://github.com/apache/airflow/pull/32709#discussion_r1282780931
##########
airflow/api/client/api_client.py:
##########
@@ -60,12 +60,13 @@ def get_pools(self):
"""Get all pools."""
raise NotImplementedError()
- def create_pool(self, name, slots, description):
+ def create_pool(self, name, slots, description, include_deferred):
Review Comment:
Not entirely sure, I think this needs a default `False` for backward compatibility.
--
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] Usiel commented on pull request #32709: Enable pools to consider deferred tasks
Posted by "Usiel (via GitHub)" <gi...@apache.org>.
Usiel commented on PR #32709:
URL: https://github.com/apache/airflow/pull/32709#issuecomment-1651437026
Regarding the two APIs:
**experimental:**
API doesn't change (except additional fields in results). `include_deferred` is set to a default `False`.
**stable/v1**:
We emit a deprecation warning if `include_deferred` is not set, but allow the request to go through (and assume `False`).
--
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] Usiel commented on pull request #32709: Enable pools to consider deferred tasks
Posted by "Usiel (via GitHub)" <gi...@apache.org>.
Usiel commented on PR #32709:
URL: https://github.com/apache/airflow/pull/32709#issuecomment-1649013243
> I think you also need to remove the required marker on the checkbox field (not sure how), otherwise the form would not allow the box to be unchecked.
Ah, that explains why the test is failing. Could have sworn I ran this test before, but neglected to run it again after changing the column to `nullable=False` on `Pool`.
I would tend to revert that decision and instead make the column nullable with a default similar to `Variable.is_encrypted` or `Connection.is_[extra_]encrypted`. This avoids hacking around Flask AppBuilder's default of making non-nullable columns required. What do you think @uranusjr?
--
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 #32709: Enable pools to consider deferred tasks
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #32709:
URL: https://github.com/apache/airflow/pull/32709
--
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 a diff in pull request #32709: Enable pools to consider deferred tasks
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32709:
URL: https://github.com/apache/airflow/pull/32709#discussion_r1288288630
##########
airflow/api/client/api_client.py:
##########
@@ -60,12 +60,13 @@ def get_pools(self):
"""Get all pools."""
raise NotImplementedError()
- def create_pool(self, name, slots, description):
+ def create_pool(self, name, slots, description, include_deferred):
Review Comment:
I took the liberty to add a fixup to remove the "experimental api" changes and rebase the change on top of latest main. I will try to convince the release manager to include that one in 2.7.0 - it would be a pity to wait till 2.8 for that one as there is clearly a need.
--
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] Usiel commented on a diff in pull request #32709: Enable pools to consider deferred tasks
Posted by "Usiel (via GitHub)" <gi...@apache.org>.
Usiel commented on code in PR #32709:
URL: https://github.com/apache/airflow/pull/32709#discussion_r1288326053
##########
airflow/api/client/api_client.py:
##########
@@ -60,12 +60,13 @@ def get_pools(self):
"""Get all pools."""
raise NotImplementedError()
- def create_pool(self, name, slots, description):
+ def create_pool(self, name, slots, description, include_deferred):
Review Comment:
> I will try to convince the release manager to include that one in 2.7.0 - it would be a pity to wait till 2.8 for that one as there is clearly a need.
Thank you, that'd be great!
Unfortunately the `pool_command` makes use of `airflow.api.client`. So if we don't want any changes to the API client, we'd also have to undo the changes for the CLI pool commands (specifically the `airflow pool set ...` cmd). I can do that now, if that's what we're going for.
--
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] boring-cyborg[bot] commented on pull request #32709: Enable pools to consider deferred tasks
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #32709:
URL: https://github.com/apache/airflow/pull/32709#issuecomment-1671243382
Awesome work, congrats on your first merged pull request! You are invited to check our [Issue Tracker](https://github.com/apache/airflow/issues) for additional contributions.
--
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] Usiel commented on a diff in pull request #32709: Enable pools to consider deferred tasks
Posted by "Usiel (via GitHub)" <gi...@apache.org>.
Usiel commented on code in PR #32709:
URL: https://github.com/apache/airflow/pull/32709#discussion_r1272919588
##########
docs/apache-airflow/authoring-and-scheduling/deferring.rst:
##########
@@ -20,7 +20,7 @@ Deferrable Operators & Triggers
Standard :doc:`Operators </core-concepts/operators>` and :doc:`Sensors <../core-concepts/sensors>` take up a full *worker slot* for the entire time they are running, even if they are idle; for example, if you only have 100 worker slots available to run Tasks, and you have 100 DAGs waiting on a Sensor that's currently running but idle, then you *cannot run anything else* - even though your entire Airflow cluster is essentially idle. ``reschedule`` mode for Sensors solves some of this, allowing Sensors to only run at fixed intervals, but it is inflexible and only allows using time as the reason to resume, not anything else.
-This is where *Deferrable Operators* come in. A deferrable operator is one that is written with the ability to suspend itself and free up the worker when it knows it has to wait, and hand off the job of resuming it to something called a *Trigger*. As a result, while it is suspended (deferred), it is not taking up a worker slot and your cluster will have a lot less resources wasted on idle Operators or Sensors.
+This is where *Deferrable Operators* come in. A deferrable operator is one that is written with the ability to suspend itself and free up the worker when it knows it has to wait, and hand off the job of resuming it to something called a *Trigger*. As a result, while it is suspended (deferred), it is not taking up a worker slot and your cluster will have a lot less resources wasted on idle Operators or Sensors. Note that by default a deferred tasks will not use up a pool slot, if you would like them to, you can change this by editing the pool in question.
Review Comment:
Oops, fixed by making everything plural
--
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] Usiel commented on a diff in pull request #32709: Enable pools to consider deferred tasks
Posted by "Usiel (via GitHub)" <gi...@apache.org>.
Usiel commented on code in PR #32709:
URL: https://github.com/apache/airflow/pull/32709#discussion_r1273030144
##########
airflow/migrations/versions/0128_2_7_0_add_include_deferred_column_to_pool.py:
##########
@@ -0,0 +1,52 @@
+#
+# 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.
+
+"""add include_deferred column to pool
+
+Revision ID: 405de8318b3a
+Revises: 788397e78828
+Create Date: 2023-07-20 04:22:21.007342
+
+"""
+
+import sqlalchemy as sa
+from alembic import op
+
+
+# revision identifiers, used by Alembic.
+revision = "405de8318b3a"
+down_revision = "788397e78828"
+branch_labels = None
+depends_on = None
+airflow_version = "2.7.0"
+
+
+def upgrade():
+ """Apply add include_deferred column to pool"""
+ with op.batch_alter_table("slot_pool") as batch_op:
+ batch_op.add_column(
+ sa.Column(
+ "include_deferred", sa.Boolean, default=False, nullable=False, server_default=sa.false()
Review Comment:
Removed it in exchange for a change in the `add_default_pool_if_not_exists` function (needed set the flag there as a result).
--
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] Usiel commented on pull request #32709: Enable pools to consider deferred tasks
Posted by "Usiel (via GitHub)" <gi...@apache.org>.
Usiel commented on PR #32709:
URL: https://github.com/apache/airflow/pull/32709#issuecomment-1649797394
Couple of places that break due to having no default value. I'll fix those and push once the full test suite completes locally. Sorry for 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] potiuk commented on a diff in pull request #32709: Enable pools to consider deferred tasks
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32709:
URL: https://github.com/apache/airflow/pull/32709#discussion_r1288269988
##########
airflow/api/client/api_client.py:
##########
@@ -60,12 +60,13 @@ def get_pools(self):
"""Get all pools."""
raise NotImplementedError()
- def create_pool(self, name, slots, description):
+ def create_pool(self, name, slots, description, include_deferred):
Review Comment:
This is old, deprecated, experimental API client that we don't use for years. As such I'd argue that we just should not change it at all.
--
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 a diff in pull request #32709: Enable pools to consider deferred tasks
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32709:
URL: https://github.com/apache/airflow/pull/32709#discussion_r1288360213
##########
airflow/api/client/api_client.py:
##########
@@ -60,12 +60,13 @@ def get_pools(self):
"""Get all pools."""
raise NotImplementedError()
- def create_pool(self, name, slots, description):
+ def create_pool(self, name, slots, description, include_deferred):
Review Comment:
Yeah. just realied taht. My bad. Actually - this was fine how it was before. I realized that the CLI command is still using tha API (Historical reason) but it is NOT the experimental API- it was internal API used in the past by the clients and we moved away from it in newer clients. There is absolutely no guarantees about this API that we give - so your change was good as it was.
resetting my change.
--
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] Usiel commented on pull request #32709: Enable pools to consider deferred tasks
Posted by "Usiel (via GitHub)" <gi...@apache.org>.
Usiel commented on PR #32709:
URL: https://github.com/apache/airflow/pull/32709#issuecomment-1651096924
It's all green again on my side. Now, the fixes in the last commit do have some effects on the public API (`/api/experimental/pools` and `/api/v1/pools`): `include_deferred` is a required field according to the model, so API calls without it will fail.
I could use some guidance on this, before I introduce more changes. Do we prefer to follow the model's constraints here or do we want to try to keep backwards compatibility?
I'd go for keeping backwards compatibility for `/api/v1/pools` and spit out a warning if the newly required `include_deferred` is not set, since the stable API docs say:
```
- APIs are designed to be backward compatible.
```
Do we want to do the same for the experimental (deprecated) API? Or should we not do any changes to it at all - except setting a default `include_deferred` to make it work?
--
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] Usiel commented on pull request #32709: Enable pools to consider deferred tasks
Posted by "Usiel (via GitHub)" <gi...@apache.org>.
Usiel commented on PR #32709:
URL: https://github.com/apache/airflow/pull/32709#issuecomment-1643661222
Rebased on `main` and fixed the failed migration and ERD pre-commit check.
--
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 a diff in pull request #32709: Enable pools to consider deferred tasks
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32709:
URL: https://github.com/apache/airflow/pull/32709#discussion_r1288360213
##########
airflow/api/client/api_client.py:
##########
@@ -60,12 +60,13 @@ def get_pools(self):
"""Get all pools."""
raise NotImplementedError()
- def create_pool(self, name, slots, description):
+ def create_pool(self, name, slots, description, include_deferred):
Review Comment:
Yeah. just realied taht. My bad. Actually - this was fine how it was before. I realized that the CLI command is still using tha API (Historical reason) but it is NOT the experimental API- it was internal API used in the past by the clients and we moved away from it in newer CLIs. There is absolutely no guarantees about this API that we give - so your change was good as it was.
resetting my change.
##########
airflow/api/client/api_client.py:
##########
@@ -60,12 +60,13 @@ def get_pools(self):
"""Get all pools."""
raise NotImplementedError()
- def create_pool(self, name, slots, description):
+ def create_pool(self, name, slots, description, include_deferred):
Review Comment:
Yeah. just realied taht. My bad. Actually - this was fine how it was before. I realized that the CLI command is still using tha API (Historical reason) but it is NOT the experimental API- it was internal API used in the past by the clients and we moved away from it in newer CLIs. There are absolutely no guarantees about this API that we give - so your change was good as it was.
resetting my change.
--
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 pull request #32709: Enable pools to consider deferred tasks
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #32709:
URL: https://github.com/apache/airflow/pull/32709#issuecomment-1671147746
Tests are failing:
```
FAILED tests/cli/commands/test_pool_command.py::TestCliPools::test_pool_list - TypeError: create_pool() got an unexpected keyword argument 'include_deferred'
FAILED tests/cli/commands/test_pool_command.py::TestCliPools::test_pool_list_with_args - IndexError: tuple index out of range
FAILED tests/cli/commands/test_pool_command.py::TestCliPools::test_pool_create - TypeError: create_pool() got an unexpected keyword argument 'include_deferred'
FAILED tests/cli/commands/test_pool_command.py::TestCliPools::test_pool_update_deferred - TypeError: create_pool() got an unexpected keyword argument 'include_deferred'
FAILED tests/cli/commands/test_pool_command.py::TestCliPools::test_pool_get - TypeError: create_pool() got an unexpected keyword argument 'include_deferred'
FAILED tests/cli/commands/test_pool_command.py::TestCliPools::test_pool_delete - TypeError: create_pool() got an unexpected keyword argument 'include_deferred'
FAILED tests/cli/commands/test_pool_command.py::TestCliPools::test_pool_import_backwards_compatibility - TypeError: create_pool() got an unexpected keyword argument 'include_deferred'
FAILED tests/cli/commands/test_pool_command.py::TestCliPools::test_pool_import_export - TypeError: create_pool() got an unexpected keyword argument 'include_deferred'
```
--
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] Usiel commented on pull request #32709: Enable pools to consider deferred tasks
Posted by "Usiel (via GitHub)" <gi...@apache.org>.
Usiel commented on PR #32709:
URL: https://github.com/apache/airflow/pull/32709#issuecomment-1651429244
> Prevent accidental merging before the OpenAPI spec is updated
Updated
--
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 pull request #32709: Enable pools to consider deferred tasks
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #32709:
URL: https://github.com/apache/airflow/pull/32709#issuecomment-1649035299
Making it nullable would cause the unchecked case to return NULL instead of FALSE, which is not a good idea. NULL in SQL is just bad. I’m pretty sure there’s a way to make a checkbox return true/false instead.
--
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 pull request #32709: Enable pools to consider deferred tasks
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #32709:
URL: https://github.com/apache/airflow/pull/32709#issuecomment-1648953987
I think you also need to remove the required marker on the checkbox field (not sure how), otherwise the form would not allow the box to be unchecked.
--
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] Usiel commented on a diff in pull request #32709: Enable pools to consider deferred tasks
Posted by "Usiel (via GitHub)" <gi...@apache.org>.
Usiel commented on code in PR #32709:
URL: https://github.com/apache/airflow/pull/32709#discussion_r1273035241
##########
airflow/models/pool.py:
##########
@@ -290,6 +312,25 @@ def scheduled_slots(self, session: Session = NEW_SESSION) -> int:
or 0
)
+ @provide_session
+ def deferred_slots(self, session: Session = NEW_SESSION) -> int:
+ """
+ Get the number of slots deferred at the moment.
+
+ :param session: SQLAlchemy ORM Session
+ :return: the number of deferred slots
+ """
+ from airflow.models.taskinstance import TaskInstance # Avoid circular import
+
+ return int(
+ session.scalar(
+ select(func.sum(TaskInstance.pool_slots))
+ .filter(TaskInstance.pool == self.pool)
+ .filter(TaskInstance.state == TaskInstanceState.DEFERRED)
Review Comment:
Committed with a fix for formatting 👍
##########
airflow/models/pool.py:
##########
@@ -290,6 +312,25 @@ def scheduled_slots(self, session: Session = NEW_SESSION) -> int:
or 0
)
+ @provide_session
+ def deferred_slots(self, session: Session = NEW_SESSION) -> int:
+ """
+ Get the number of slots deferred at the moment.
+
+ :param session: SQLAlchemy ORM Session
+ :return: the number of deferred slots
+ """
+ from airflow.models.taskinstance import TaskInstance # Avoid circular import
+
+ return int(
+ session.scalar(
+ select(func.sum(TaskInstance.pool_slots))
+ .filter(TaskInstance.pool == self.pool)
+ .filter(TaskInstance.state == TaskInstanceState.DEFERRED)
Review Comment:
Committed with a fix for formatting 👍
--
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 pull request #32709: Enable pools to consider deferred tasks
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #32709:
URL: https://github.com/apache/airflow/pull/32709#issuecomment-1661678038
No nullable. Null in the database field simply does not make sense. I think the idea above is to make the _form_ field not required and fill in the default for the database.
--
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