pierrejeambrun commented on code in PR #61140:
URL: https://github.com/apache/airflow/pull/61140#discussion_r2742591601
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_pools.py:
##########
@@ -498,6 +491,52 @@ def test_should_respond_200(self, test_client, session,
body, expected_status_co
assert session.scalar(select(func.count()).select_from(Pool)) ==
n_pools + 1
check_last_log(session, dag_id=None, event="post_pool",
logical_date=None)
+ def test_post_pool_allows_unlimited_slots(self, test_client, session):
+ """POST /pools should accept slots = -1 (unlimited)."""
+ self.create_pools()
+ n_pools = session.scalar(select(func.count()).select_from(Pool))
+
+ response = test_client.post(
+ "/pools",
+ json={
+ "name": "unlimited_pool",
+ "slots": -1,
+ "description": "Unlimited pool",
+ "include_deferred": False,
+ },
+ )
+
+ assert response.status_code == 201
+ body = response.json()
+ assert body["name"] == "unlimited_pool"
+ assert body["slots"] == -1
+ assert body.get("open_slots") in (None, float("inf"),"Infinity")
+ assert session.scalar(select(func.count()).select_from(Pool)) ==
n_pools + 1
+ check_last_log(session, dag_id=None, event="post_pool",
logical_date=None)
+
+
+ def test_post_pool_accepts_infinity_string(self, test_client, session):
+ """POST /pools should accept 'infinity' string for slots and coerce to
-1."""
Review Comment:
We don't need to allow 'infinity'. User should pass '-1' for that and it
should be documented in the endpoint spec.
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/pools.py:
##########
@@ -34,23 +34,37 @@ def _call_function(function: Callable[[], int]) -> int:
return function()
+PoolSlots = Annotated[
+ int,
+ BeforeValidator(lambda v: -1 if isinstance(v, str) and v.lower() in
{"inf", "infinity", "∞"} else v),
+ Field(ge=-1, description="Number of slots. Use -1 for unlimited."),
+]
+
+
class BasePool(BaseModel):
"""Base serializer for Pool."""
pool: str = Field(serialization_alias="name")
- slots: PositiveInt
+ slots: PoolSlots
description: str | None = Field(default=None)
include_deferred: bool
+def _sanitize_open_slots(value: int | float) -> int | None:
Review Comment:
'None' isn't explicit, we probably want to return '-1' instead.
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/pools.py:
##########
@@ -34,23 +34,37 @@ def _call_function(function: Callable[[], int]) -> int:
return function()
+PoolSlots = Annotated[
+ int,
+ BeforeValidator(lambda v: -1 if isinstance(v, str) and v.lower() in
{"inf", "infinity", "∞"} else v),
+ Field(ge=-1, description="Number of slots. Use -1 for unlimited."),
+]
+
+
class BasePool(BaseModel):
"""Base serializer for Pool."""
pool: str = Field(serialization_alias="name")
- slots: PositiveInt
+ slots: PoolSlots
description: str | None = Field(default=None)
include_deferred: bool
+def _sanitize_open_slots(value: int | float) -> int | None:
+ # JSON does not support Infinity
+ if isinstance(value, float) and value == float("inf"):
+ return None
+ return value
+
+
class PoolResponse(BasePool):
"""Pool serializer for responses."""
occupied_slots: Annotated[int, BeforeValidator(_call_function)]
running_slots: Annotated[int, BeforeValidator(_call_function)]
queued_slots: Annotated[int, BeforeValidator(_call_function)]
scheduled_slots: Annotated[int, BeforeValidator(_call_function)]
- open_slots: Annotated[int, BeforeValidator(_call_function)]
+ open_slots: Annotated[int | float, BeforeValidator(lambda v:
_sanitize_open_slots(_call_function(v)))]
Review Comment:
Why 'or float' ? the beforevalidator will return None or int ?
```suggestion
open_slots: Annotated[int, BeforeValidator(lambda v:
_sanitize_open_slots(_call_function(v)))]
```
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_pools.py:
##########
@@ -498,6 +491,52 @@ def test_should_respond_200(self, test_client, session,
body, expected_status_co
assert session.scalar(select(func.count()).select_from(Pool)) ==
n_pools + 1
check_last_log(session, dag_id=None, event="post_pool",
logical_date=None)
+ def test_post_pool_allows_unlimited_slots(self, test_client, session):
+ """POST /pools should accept slots = -1 (unlimited)."""
+ self.create_pools()
+ n_pools = session.scalar(select(func.count()).select_from(Pool))
+
+ response = test_client.post(
+ "/pools",
+ json={
+ "name": "unlimited_pool",
+ "slots": -1,
+ "description": "Unlimited pool",
+ "include_deferred": False,
+ },
+ )
+
+ assert response.status_code == 201
+ body = response.json()
+ assert body["name"] == "unlimited_pool"
+ assert body["slots"] == -1
+ assert body.get("open_slots") in (None, float("inf"),"Infinity")
Review Comment:
assert the real value here
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/pools.py:
##########
@@ -34,23 +34,37 @@ def _call_function(function: Callable[[], int]) -> int:
return function()
+PoolSlots = Annotated[
+ int,
+ BeforeValidator(lambda v: -1 if isinstance(v, str) and v.lower() in
{"inf", "infinity", "∞"} else v),
+ Field(ge=-1, description="Number of slots. Use -1 for unlimited."),
+]
+
+
class BasePool(BaseModel):
"""Base serializer for Pool."""
pool: str = Field(serialization_alias="name")
- slots: PositiveInt
+ slots: PoolSlots
description: str | None = Field(default=None)
include_deferred: bool
+def _sanitize_open_slots(value: int | float) -> int | None:
Review Comment:
Type is not good here I think for a before validator. It should receive Any
I believe.
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/pools.py:
##########
@@ -34,23 +34,37 @@ def _call_function(function: Callable[[], int]) -> int:
return function()
+PoolSlots = Annotated[
+ int,
+ BeforeValidator(lambda v: -1 if isinstance(v, str) and v.lower() in
{"inf", "infinity", "∞"} else v),
+ Field(ge=-1, description="Number of slots. Use -1 for unlimited."),
+]
+
+
class BasePool(BaseModel):
"""Base serializer for Pool."""
pool: str = Field(serialization_alias="name")
- slots: PositiveInt
+ slots: PoolSlots
description: str | None = Field(default=None)
include_deferred: bool
+def _sanitize_open_slots(value: int | float) -> int | None:
Review Comment:
In any case if we chose 'None' or '-1' we need to document that in the spec.
"`-1` represents infinity.... "
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/pools.py:
##########
@@ -34,23 +34,37 @@ def _call_function(function: Callable[[], int]) -> int:
return function()
+PoolSlots = Annotated[
+ int,
+ BeforeValidator(lambda v: -1 if isinstance(v, str) and v.lower() in
{"inf", "infinity", "∞"} else v),
+ Field(ge=-1, description="Number of slots. Use -1 for unlimited."),
+]
+
+
class BasePool(BaseModel):
"""Base serializer for Pool."""
pool: str = Field(serialization_alias="name")
- slots: PositiveInt
+ slots: PoolSlots
description: str | None = Field(default=None)
include_deferred: bool
+def _sanitize_open_slots(value: int | float) -> int | None:
+ # JSON does not support Infinity
+ if isinstance(value, float) and value == float("inf"):
+ return None
+ return value
Review Comment:
Does that mean that the API will return 'None' when the slots is infinity ?
I think that's not in line with the spec. Can we instead return `-1` ?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
