-
Notifications
You must be signed in to change notification settings - Fork 310
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
Add shared_memory to task with extended resources #3096
base: master
Are you sure you want to change the base?
Changes from all commits
ac61755
e3221e2
65dda33
aecce00
eadab88
0dc82b2
ad72c5d
567e691
44def02
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -1,9 +1,13 @@ | ||||||||
from dataclasses import dataclass, fields | ||||||||
from typing import List, Optional, Union | ||||||||
from typing import Literal as L | ||||||||
|
||||||||
from flyteidl.core import tasks_pb2 | ||||||||
from kubernetes.client import V1Container, V1PodSpec, V1ResourceRequirements | ||||||||
from mashumaro.mixins.json import DataClassJSONMixin | ||||||||
|
||||||||
from flytekit.core.constants import SHARED_MEMORY_MOUNT_NAME, SHARED_MEMORY_MOUNT_PATH | ||||||||
from flytekit.extras.accelerators import BaseAccelerator | ||||||||
from flytekit.models import task as task_models | ||||||||
|
||||||||
|
||||||||
|
@@ -102,6 +106,35 @@ def convert_resources_to_resource_model( | |||||||
return task_models.Resources(requests=request_entries, limits=limit_entries) | ||||||||
|
||||||||
|
||||||||
def construct_extended_resources( | ||||||||
*, | ||||||||
accelerator: Optional[BaseAccelerator] = None, | ||||||||
shared_memory: Optional[Union[L[True], str]] = None, | ||||||||
) -> Optional[tasks_pb2.ExtendedResources]: | ||||||||
"""Convert public extended resources to idl. | ||||||||
|
||||||||
:param accelerator: The accelerator to use for this task. | ||||||||
:param shared_memory: If True, then shared memory will be attached to the container where the size is equal | ||||||||
to the allocated memory. If str, then the shared memory is set to that size. | ||||||||
""" | ||||||||
kwargs = {} | ||||||||
if accelerator is not None: | ||||||||
kwargs["gpu_accelerator"] = accelerator.to_flyte_idl() | ||||||||
if isinstance(shared_memory, str) or shared_memory is True: | ||||||||
if shared_memory is True: | ||||||||
shared_memory = None | ||||||||
kwargs["shared_memory"] = tasks_pb2.SharedMemory( | ||||||||
mount_name=SHARED_MEMORY_MOUNT_NAME, | ||||||||
mount_path=SHARED_MEMORY_MOUNT_PATH, | ||||||||
size_limit=shared_memory, | ||||||||
) | ||||||||
|
||||||||
if not kwargs: | ||||||||
return None | ||||||||
|
||||||||
return tasks_pb2.ExtendedResources(**kwargs) | ||||||||
|
||||||||
|
||||||||
def pod_spec_from_resources( | ||||||||
primary_container_name: Optional[str] = None, | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Parameter name and type change impact
The parameter name change from Code suggestionCheck the AI-generated fix before applying
Suggested change
Code Review Run #31b042 Is this a valid issue, or was it incorrectly flagged by the Agent?
|
||||||||
requests: Optional[Resources] = None, | ||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -8,7 +8,9 @@ | |||||||||||
from flytekit.core.resources import ( | ||||||||||||
pod_spec_from_resources, | ||||||||||||
convert_resources_to_resource_model, | ||||||||||||
construct_extended_resources, | ||||||||||||
) | ||||||||||||
from flytekit.extras.accelerators import T4 | ||||||||||||
|
||||||||||||
_ResourceName = _task_models.Resources.ResourceName | ||||||||||||
|
||||||||||||
|
@@ -155,3 +157,18 @@ 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 | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider type conversion before assertion comparison
The assertion Code suggestionCheck the AI-generated fix before applying
Suggested change
Code Review Run #31b042 Is this a valid issue, or was it incorrectly flagged by the Agent?
|
||||||||||||
|
||||||||||||
|
||||||||||||
@pytest.mark.parametrize("shared_memory", [None, False]) | ||||||||||||
def test_construct_extended_resources_shared_memory_none(shared_memory): | ||||||||||||
resources = construct_extended_resources(shared_memory=shared_memory) | ||||||||||||
Comment on lines
+162
to
+164
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider consolidating redundant test cases
Consider consolidating the test cases for Code suggestionCheck the AI-generated fix before applying
Suggested change
Code Review Run #3d3e41 Is this a valid issue, or was it incorrectly flagged by the Agent?
|
||||||||||||
assert resources is None | ||||||||||||
|
||||||||||||
|
||||||||||||
@pytest.mark.parametrize("shared_memory, expected_size_limit", [ | ||||||||||||
("2Gi", "2Gi"), | ||||||||||||
(True, ""), | ||||||||||||
]) | ||||||||||||
def test_construct_extended_resources_shared_memory(shared_memory, expected_size_limit): | ||||||||||||
resources = construct_extended_resources(shared_memory=shared_memory) | ||||||||||||
assert resources.shared_memory.size_limit == expected_size_limit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a more secure temporary file location instead of hardcoding '/dev/shm'. The shared memory directory could potentially be accessed by other processes on the system. Consider using 'tempfile.gettempdir()' to get a secure temporary directory location.
Code suggestion
Code Review Run #3d3e41
Is this a valid issue, or was it incorrectly flagged by the Agent?