Skip to content

[Serve] Rename loop variable to avoid shadowing dataclasses.field#63226

Merged
abrarsheikh merged 3 commits into
ray-project:masterfrom
XuQianJin-Stars:fix/schema-field-shadowing
May 15, 2026
Merged

[Serve] Rename loop variable to avoid shadowing dataclasses.field#63226
abrarsheikh merged 3 commits into
ray-project:masterfrom
XuQianJin-Stars:fix/schema-field-shadowing

Conversation

@XuQianJin-Stars
Copy link
Copy Markdown
Contributor

Description

In python/ray/serve/schema.py, the module imports field from dataclasses at the top of the file:

from dataclasses import dataclass, field

and uses it later as a default-factory helper:

proxies: Dict[str, ProxyStatus] = field(default_factory=dict)
applications: Dict[str, ApplicationStatusOverview] = field(default_factory=dict)

However, two places inside DeploymentSchema use field as the name of a loop / comprehension variable:

  • validate_gang_scheduling_and_num_replicasfor field in ["min_replicas", "max_replicas", "initial_replicas"]:
  • _get_user_configured_option_names{field for field in self.model_fields_set ...}

This shadows the module-level dataclasses.field inside those function scopes. It does not cause a runtime bug today (both usages are self-contained), but it:

  • triggers a pyflakes warning: "import 'field' from line 4 shadowed by loop variable",
  • makes the code harder to read — the loop variable actually holds a field name string, not a dataclass field object,
  • is a latent footgun if anyone later adds a field(...) call inside these functions.

This PR renames the two loop variables to field_name, which:

  • removes the shadowing (and the pyflakes warning),
  • more accurately describes what the variable holds,
  • is a pure rename with no behavior change.

Diff summary

Minimal: 6 insertions / 6 deletions in a single file.

-                for field in ["min_replicas", "max_replicas", "initial_replicas"]:
-                    val = autoscaling_config.get(field)
+                for field_name in ["min_replicas", "max_replicas", "initial_replicas"]:
+                    val = autoscaling_config.get(field_name)
                     if val is not None and val % gang_config.gang_size != 0:
                         raise ValueError(
-                            f"autoscaling_config.{field} ({val}) must be a "
+                            f"autoscaling_config.{field_name} ({val}) must be a "
                             f"multiple of gang_size ({gang_config.gang_size})."
                         )

-        return {
-            field
-            for field in self.model_fields_set
-            if getattr(self, field) is not DEFAULT.VALUE
-        }
+        return {
+            field_name
+            for field_name in self.model_fields_set
+            if getattr(self, field_name) is not DEFAULT.VALUE
+        }

Verification

  • python -m py_compile python/ray/serve/schema.py
  • python -m pyflakes python/ray/serve/schema.py ✅ (no warnings)
  • ruff check python/ray/serve/schema.py ✅ (All checks passed)
  • black --check python/ray/serve/schema.py ✅ (would be left unchanged)

Related issues

None — pure code hygiene / lint cleanup, no user-visible behavior change.

Additional information

  • No public API changes.
  • No tests added: this is a pure rename of two local loop variables; existing tests cover the surrounding logic unchanged.
  • No docs changes needed.

Checks

  • I've signed off every commit (DCO).
  • I've run the formatters and linters locally (ruff, black, pyflakes, py_compile).
  • Diff is minimal and strictly a rename.
  • Release tests — not applicable (no behavior change).
  • Documentation — not applicable.

In schema.py, two loops used 'field' as the iteration variable, which shadows the module-level 'dataclasses.field' import. Rename them to 'field_name' for clarity and to avoid accidental shadowing. No behavior change.

Signed-off-by: forwardxu <forwardxu@apache.org>
@XuQianJin-Stars XuQianJin-Stars requested a review from a team as a code owner May 8, 2026 11:51
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request renames the variable field to field_name within python/ray/serve/schema.py for better clarity. The review feedback identifies a potential AttributeError in the gang scheduling validation logic, as autoscaling_config may be a Pydantic model rather than a dictionary, and suggests using getattr to safely access the configuration fields.

for field in ["min_replicas", "max_replicas", "initial_replicas"]:
val = autoscaling_config.get(field)
for field_name in ["min_replicas", "max_replicas", "initial_replicas"]:
val = autoscaling_config.get(field_name)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The code assumes autoscaling_config is a dictionary by calling .get(). However, the DeploymentSchema allows autoscaling_config to be either a Dict or an AutoscalingConfig object (see line 352). If an AutoscalingConfig instance is passed to the constructor, this line will raise an AttributeError because Pydantic BaseModel does not have a .get() method.

Consider handling both cases to ensure robustness. Note that line 519 (currently outside the diff) appears to have the same issue.

Suggested change
val = autoscaling_config.get(field_name)
val = autoscaling_config.get(field_name) if isinstance(autoscaling_config, dict) else getattr(autoscaling_config, field_name, None)

In the "before" mode validator validate_gang_scheduling_config, autoscaling_config may be either a dict (from raw input) or an AutoscalingConfig instance (if already constructed). Calling .get() on an AutoscalingConfig instance would raise AttributeError. Normalize it to a dict via model_dump() once, so both subsequent lookups work uniformly regardless of the input type.

Signed-off-by: forwardxu <forwardxu@apache.org>
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit 2af82f8. Configure here.

Comment thread python/ray/serve/schema.py Outdated
…gang validator

model_dump() without exclude_unset=True emits all fields with their defaults (e.g. min_replicas=1, max_replicas=1), so passing an AutoscalingConfig instance would trigger gang-size multiple validation on default values the user never explicitly set, while an equivalent dict (where unset keys are simply absent) would skip those checks. Use model_dump(exclude_unset=True) to only validate user-set fields, matching how AutoscalingConfig is handled elsewhere in the codebase (e.g. ray/serve/_private/config.py).

Signed-off-by: forwardxu <forwardxu@apache.org>
@ray-gardener ray-gardener Bot added serve Ray Serve Related Issue community-contribution Contributed by the community labels May 8, 2026
@abrarsheikh abrarsheikh added the go add ONLY when ready to merge, run all tests label May 8, 2026
@abrarsheikh abrarsheikh merged commit dc59cfb into ray-project:master May 15, 2026
10 checks passed
TruongQuangPhat pushed a commit to cyhapun/ray-fix-issue that referenced this pull request May 27, 2026
…y-project#63226)

## Description

In `python/ray/serve/schema.py`, the module imports `field` from
`dataclasses` at the top of the file:

    from dataclasses import dataclass, field

and uses it later as a default-factory helper:

    proxies: Dict[str, ProxyStatus] = field(default_factory=dict)
applications: Dict[str, ApplicationStatusOverview] =
field(default_factory=dict)

However, two places inside `DeploymentSchema` use `field` as the name of
a loop / comprehension variable:

- `validate_gang_scheduling_and_num_replicas` — `for field in
["min_replicas", "max_replicas", "initial_replicas"]:`
- `_get_user_configured_option_names` — `{field for field in
self.model_fields_set ...}`

This shadows the module-level `dataclasses.field` inside those function
scopes. It does not cause a runtime bug today (both usages are
self-contained), but it:

- triggers a `pyflakes` warning: *"import 'field' from line 4 shadowed
by loop variable"*,
- makes the code harder to read — the loop variable actually holds a
*field name string*, not a `dataclass` field object,
- is a latent footgun if anyone later adds a `field(...)` call inside
these functions.

This PR renames the two loop variables to `field_name`, which:

- removes the shadowing (and the pyflakes warning),
- more accurately describes what the variable holds,
- is a pure rename with **no behavior change**.

### Diff summary

Minimal: 6 insertions / 6 deletions in a single file.

- for field in ["min_replicas", "max_replicas", "initial_replicas"]:
    -                    val = autoscaling_config.get(field)
+ for field_name in ["min_replicas", "max_replicas",
"initial_replicas"]:
    +                    val = autoscaling_config.get(field_name)
if val is not None and val % gang_config.gang_size != 0:
                             raise ValueError(
- f"autoscaling_config.{field} ({val}) must be a "
+ f"autoscaling_config.{field_name} ({val}) must be a "
f"multiple of gang_size ({gang_config.gang_size})."
                             )

    -        return {
    -            field
    -            for field in self.model_fields_set
    -            if getattr(self, field) is not DEFAULT.VALUE
    -        }
    +        return {
    +            field_name
    +            for field_name in self.model_fields_set
    +            if getattr(self, field_name) is not DEFAULT.VALUE
    +        }

### Verification

- `python -m py_compile python/ray/serve/schema.py` ✅
- `python -m pyflakes python/ray/serve/schema.py` ✅ (no warnings)
- `ruff check python/ray/serve/schema.py` ✅ (All checks passed)
- `black --check python/ray/serve/schema.py` ✅ (would be left unchanged)

## Related issues

None — pure code hygiene / lint cleanup, no user-visible behavior
change.

## Additional information

- No public API changes.
- No tests added: this is a pure rename of two local loop variables;
existing tests cover the surrounding logic unchanged.
- No docs changes needed.

---

#### Checks

- [x] I've signed off every commit (DCO).
- [x] I've run the formatters and linters locally (`ruff`, `black`,
`pyflakes`, `py_compile`).
- [x] Diff is minimal and strictly a rename.
- [ ] Release tests — not applicable (no behavior change).
- [ ] Documentation — not applicable.

---------

Signed-off-by: forwardxu <forwardxu@apache.org>
Signed-off-by: phattruong <23120318@student.hcmus.edu.vn>
Neelansh-Khare pushed a commit to Neelansh-Khare/ray-clone that referenced this pull request Jun 5, 2026
…y-project#63226)

## Description

In `python/ray/serve/schema.py`, the module imports `field` from
`dataclasses` at the top of the file:

    from dataclasses import dataclass, field

and uses it later as a default-factory helper:

    proxies: Dict[str, ProxyStatus] = field(default_factory=dict)
applications: Dict[str, ApplicationStatusOverview] =
field(default_factory=dict)

However, two places inside `DeploymentSchema` use `field` as the name of
a loop / comprehension variable:

- `validate_gang_scheduling_and_num_replicas` — `for field in
["min_replicas", "max_replicas", "initial_replicas"]:`
- `_get_user_configured_option_names` — `{field for field in
self.model_fields_set ...}`

This shadows the module-level `dataclasses.field` inside those function
scopes. It does not cause a runtime bug today (both usages are
self-contained), but it:

- triggers a `pyflakes` warning: *"import 'field' from line 4 shadowed
by loop variable"*,
- makes the code harder to read — the loop variable actually holds a
*field name string*, not a `dataclass` field object,
- is a latent footgun if anyone later adds a `field(...)` call inside
these functions.

This PR renames the two loop variables to `field_name`, which:

- removes the shadowing (and the pyflakes warning),
- more accurately describes what the variable holds,
- is a pure rename with **no behavior change**.

### Diff summary

Minimal: 6 insertions / 6 deletions in a single file.

- for field in ["min_replicas", "max_replicas", "initial_replicas"]:
    -                    val = autoscaling_config.get(field)
+ for field_name in ["min_replicas", "max_replicas",
"initial_replicas"]:
    +                    val = autoscaling_config.get(field_name)
if val is not None and val % gang_config.gang_size != 0:
                             raise ValueError(
- f"autoscaling_config.{field} ({val}) must be a "
+ f"autoscaling_config.{field_name} ({val}) must be a "
f"multiple of gang_size ({gang_config.gang_size})."
                             )

    -        return {
    -            field
    -            for field in self.model_fields_set
    -            if getattr(self, field) is not DEFAULT.VALUE
    -        }
    +        return {
    +            field_name
    +            for field_name in self.model_fields_set
    +            if getattr(self, field_name) is not DEFAULT.VALUE
    +        }

### Verification

- `python -m py_compile python/ray/serve/schema.py` ✅
- `python -m pyflakes python/ray/serve/schema.py` ✅ (no warnings)
- `ruff check python/ray/serve/schema.py` ✅ (All checks passed)
- `black --check python/ray/serve/schema.py` ✅ (would be left unchanged)

## Related issues

None — pure code hygiene / lint cleanup, no user-visible behavior
change.

## Additional information

- No public API changes.
- No tests added: this is a pure rename of two local loop variables;
existing tests cover the surrounding logic unchanged.
- No docs changes needed.

---

#### Checks

- [x] I've signed off every commit (DCO).
- [x] I've run the formatters and linters locally (`ruff`, `black`,
`pyflakes`, `py_compile`).
- [x] Diff is minimal and strictly a rename.
- [ ] Release tests — not applicable (no behavior change).
- [ ] Documentation — not applicable.

---------

Signed-off-by: forwardxu <forwardxu@apache.org>
Signed-off-by: Neelansh Khare <kharen@uci.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-contribution Contributed by the community go add ONLY when ready to merge, run all tests serve Ray Serve Related Issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants