Copilot commented on code in PR #66979:
URL: https://github.com/apache/airflow/pull/66979#discussion_r3246183561
##########
pyproject.toml:
##########
@@ -653,6 +653,7 @@ extend-select = [
"B004", # Checks for use of hasattr(x, "__call__") and replaces it with
callable(x)
"B006", # Checks for uses of mutable objects as function argument defaults.
"B007", # Checks for unused variables in the loop
+ "B008", # Do not perform function call in argument defaults (use
extend-immutable-calls for FastAPI DI)
Review Comment:
Enabling B008 here appears to make the existing lint configuration fail
because not all current function-call defaults are either fixed or exempted.
For example, there are still many `conf.getboolean("operators",
"default_deferrable", ...)` defaults in providers and FastAPI `Header(...)`
defaults in `providers/edge3/src/airflow/providers/edge3/worker_api/auth.py`,
while `Header` is not in `extend-immutable-calls`. Please either fix/exempt all
remaining violations before enabling the rule or scope the rule so CI can pass.
##########
task-sdk/src/airflow/sdk/bases/sensor.py:
##########
@@ -128,6 +128,8 @@ def __init__(
super().__init__(**kwargs)
self.poke_interval =
self._coerce_poke_interval(poke_interval).total_seconds()
self.soft_fail = soft_fail
+ if timeout is None:
+ timeout = conf.getfloat("sensors", "default_timeout")
Review Comment:
This changes the public defaulting behavior for omitted `timeout`, but the
existing sensor tests always inject a timeout in the fixture and do not cover
reading the current `sensors.default_timeout` at instantiation. Please add a
regression test that changes the config before constructing the sensor and
verifies the default is picked up.
##########
providers/google/src/airflow/providers/google/cloud/transfers/s3_to_gcs.py:
##########
@@ -178,7 +178,11 @@ def __init__(
self.verify = verify
self.gzip = gzip
self.google_impersonation_chain = google_impersonation_chain
- self.deferrable = deferrable
+ self.deferrable = (
+ deferrable
+ if deferrable is not None
+ else conf.getboolean("operators", "default_deferrable",
fallback=False)
+ )
Review Comment:
The default for `deferrable` now depends on `operators.default_deferrable`
at operator construction time, but the existing S3ToGCS tests only cover
explicit values. Please add a regression test for omitted `deferrable` with the
config set, and for explicit `False` overriding a true config value.
##########
providers/google/src/airflow/providers/google/cloud/hooks/vertex_ai/ray.py:
##########
@@ -115,7 +115,7 @@ def create_ray_cluster(
"""
aiplatform.init(project=project_id, location=location,
credentials=self.get_credentials())
cluster_path = vertex_ray.create_ray_cluster(
- head_node_type=head_node_type,
+ head_node_type=head_node_type or resources.Resources(),
Review Comment:
This fallback uses truthiness, so an explicit falsy `head_node_type` value
is silently replaced with a new default `Resources()` instead of being
forwarded as before. The operator uses an `is not None` check for the same
parameter; the hook should do the same to preserve explicit values and avoid
masking invalid inputs.
##########
task-sdk/src/airflow/sdk/definitions/operator_resources.py:
##########
@@ -125,15 +125,15 @@ class Resources:
def __init__(
self,
- cpus=conf.getint("operators", "default_cpus"),
- ram=conf.getint("operators", "default_ram"),
- disk=conf.getint("operators", "default_disk"),
- gpus=conf.getint("operators", "default_gpus"),
+ cpus=None,
+ ram=None,
+ disk=None,
+ gpus=None,
):
- self.cpus = CpuResource(cpus)
- self.ram = RamResource(ram)
- self.disk = DiskResource(disk)
- self.gpus = GpuResource(gpus)
+ self.cpus = CpuResource(cpus if cpus is not None else
conf.getint("operators", "default_cpus"))
+ self.ram = RamResource(ram if ram is not None else
conf.getint("operators", "default_ram"))
+ self.disk = DiskResource(disk if disk is not None else
conf.getint("operators", "default_disk"))
+ self.gpus = GpuResource(gpus if gpus is not None else
conf.getint("operators", "default_gpus"))
Review Comment:
These defaults now come from `conf` at construction time, but the existing
`Resources` tests only cover explicitly supplied values. Please add coverage
for omitted resource fields reading the current config values, including falsy
explicit values such as `0` still being preserved.
--
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]
