Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename ephemeral_storage to disk for simplicity #3095

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 28 additions & 4 deletions flytekit/core/resources.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from dataclasses import dataclass, fields
import warnings
from dataclasses import InitVar, dataclass, fields
from typing import List, Optional, Union

from kubernetes.client import V1Container, V1PodSpec, V1ResourceRequirements
Expand All @@ -19,8 +20,9 @@ class Resources(DataClassJSONMixin):
Resources(cpu=0.5, mem=1024) # This is 500m CPU and 1 KB of memory

# For Kubernetes-based tasks, pods use ephemeral local storage for scratch space, caching, and for logs.
# The property has been renamed to disk for simplicity, while still retaining its ephemeral nature.
# This allocates 1Gi of such local storage.
Resources(ephemeral_storage="1Gi")
Resources(disk="1Gi")

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider maintaining backward compatibility for parameters

Consider maintaining backward compatibility by keeping both disk and ephemeral_storage parameters functional. While renaming to disk improves clarity, some existing code may still use ephemeral_storage. Consider handling both parameters and adding a deprecation warning for ephemeral_storage.

Code suggestion
Check the AI-generated fix before applying
 -    disk: InitVar[Optional[Union[str, int]]] = None
 -    ephemeral_storage: Optional[Union[str, int]] = None
 +    disk: Optional[Union[str, int]] = None
 +    ephemeral_storage: InitVar[Optional[Union[str, int]]] = None
 
      def __post_init__(self, disk):
 +        if disk is not None:
 +            self.disk = disk
 +        if self.ephemeral_storage is not None:
 +            warnings.warn('The ephemeral_storage parameter is deprecated. Please use disk instead.', DeprecationWarning)
 +            if self.disk is None:
 +                self.disk = self.ephemeral_storage

Code Review Run #c7015e


Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged

.. note::

Expand All @@ -33,9 +35,10 @@ class Resources(DataClassJSONMixin):
cpu: Optional[Union[str, int, float]] = None
mem: Optional[Union[str, int]] = None
gpu: Optional[Union[str, int]] = None
disk: InitVar[Optional[Union[str, int]]] = None
ephemeral_storage: Optional[Union[str, int]] = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider consistent type annotation for resources

Consider using a consistent type annotation pattern for resource fields. While disk is marked as InitVar, other similar resource fields (cpu, mem, gpu, ephemeral_storage) are not. This inconsistency might lead to confusion in how these fields are processed.

Code suggestion
Check the AI-generated fix before applying
 -    cpu: Optional[Union[str, int, float]] = None
 -    mem: Optional[Union[str, int]] = None
 -    gpu: Optional[Union[str, int]] = None
 -    disk: InitVar[Optional[Union[str, int]]] = None
 -    ephemeral_storage: Optional[Union[str, int]] = None
 +    cpu: InitVar[Optional[Union[str, int, float]]] = None
 +    mem: InitVar[Optional[Union[str, int]]] = None
 +    gpu: InitVar[Optional[Union[str, int]]] = None
 +    disk: InitVar[Optional[Union[str, int]]] = None
 +    ephemeral_storage: InitVar[Optional[Union[str, int]]] = None

Code Review Run #c7015e


Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged


def __post_init__(self):
def __post_init__(self, disk):
def _check_cpu(value):
if value is None:
return
Expand All @@ -51,7 +54,28 @@ def _check_others(value):
_check_cpu(self.cpu)
_check_others(self.mem)
_check_others(self.gpu)
_check_others(self.ephemeral_storage)

# Rename the `ephemeral_storage` parameter to `disk` for simplicity.
# Currently, both `disk` and `ephemeral_storage` are supported to maintain backward compatibility.
# For more details, please refer to https://github.com/flyteorg/flyte/issues/6142.
if disk is not None and self.ephemeral_storage is not None:
raise ValueError(
"Cannot specify both disk and ephemeral_storage."
"Please use disk because ephemeral_storage is deprecated and will be removed in the future."
)
elif self.ephemeral_storage is not None:
warnings.warn(
"ephemeral_storage is deprecated and will be removed in the future. Please use disk instead.",
DeprecationWarning,
)
self.disk = self.ephemeral_storage
else:
self.disk = disk

# This alias ensures backward compatibility, allowing `ephemeral_storage`
# to mirror the value of `disk` for seamless attribute access
self.ephemeral_storage = self.disk
_check_others(self.disk)


@dataclass
Expand Down
24 changes: 24 additions & 0 deletions tests/flytekit/unit/core/test_resources.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,3 +155,27 @@ def test_pod_spec_from_resources_requests_set():
)
pod_spec = pod_spec_from_resources(primary_container_name=primary_container_name, requests=requests, limits=limits)
assert expected_pod_spec == pod_spec


def test_disk():
# Define expected warning and error messages
WARN_MSG = "ephemeral_storage is deprecated and will be removed in the future. Please use disk instead."
ERR_MSG = (
"Cannot specify both disk and ephemeral_storage."
"Please use disk because ephemeral_storage is deprecated and will be removed in the future."
)

# Specify both disk and ephemeral_storage
with pytest.raises(ValueError, match=ERR_MSG):
resources = Resources(cpu="1", mem="1Gi", gpu="1", disk="1Gi", ephemeral_storage="1Gi")

# Specify only ephemeral_storage
with pytest.warns(DeprecationWarning, match=WARN_MSG):
resources = Resources(cpu="1", mem="1Gi", gpu="1", ephemeral_storage="1Gi")
assert resources.disk == "1Gi"
assert resources.ephemeral_storage == "1Gi"

# Specify only disk
resources = Resources(cpu="1", mem="1Gi", gpu="1", disk="1Gi")
assert resources.disk == "1Gi"
assert resources.ephemeral_storage == "1Gi"
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider removing redundant test assertions

Consider removing redundant assertions since disk and ephemeral_storage are expected to have the same value. A single assertion would be sufficient to verify the behavior.

Code suggestion
Check the AI-generated fix before applying
Suggested change
assert resources.ephemeral_storage == "1Gi"

Code Review Run #c7015e


Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged

Loading