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.

Sign up for GitHub

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

Jump to bottom

fix: resource decorator step functions issue #1610

Merged
merged 3 commits into from
Oct 24, 2023
Merged

fix: resource decorator step functions issue #1610

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1d921d9
remove batch attribute computation as unnecessary for step-functions
saikonen Oct 24, 2023
c7f6702
remove unused imports
saikonen Oct 24, 2023
30992be
fix shared_memory and memory value handling for step-functions
saikonen Oct 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions metaflow/plugins/aws/batch/batch_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ def _register_job_definition(
if shared_memory is not None:
if not (
isinstance(shared_memory, (int, unicode, basestring))
and int(shared_memory) > 0
and int(float(shared_memory)) > 0
):
raise BatchJobException(
"Invalid shared memory size value ({}); "
Expand All @@ -225,7 +225,7 @@ def _register_job_definition(
else:
job_definition["containerProperties"]["linuxParameters"][
"sharedMemorySize"
] = int(shared_memory)
] = int(float(shared_memory))
if swappiness is not None:
if not (
isinstance(swappiness, (int, unicode, basestring))
Expand Down Expand Up @@ -298,7 +298,7 @@ def _register_job_definition(
)
else:
# default tmpfs behavior - https://man7.org/linux/man-pages/man5/tmpfs.5.html
tmpfs_size = int(memory) / 2
tmpfs_size = int(float(memory)) / 2

job_definition["containerProperties"]["linuxParameters"]["tmpfs"] = [
{
Expand Down
15 changes: 2 additions & 13 deletions metaflow/plugins/aws/step_functions/step_functions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,11 @@
import random
import string
import sys
import time
import uuid
from collections import defaultdict

from metaflow import R
from metaflow.decorators import flow_decorators
from metaflow.exception import MetaflowException, MetaflowInternalError
from metaflow.exception import MetaflowException
from metaflow.metaflow_config import (
EVENTS_SFN_ACCESS_IAM_ROLE,
S3_ENDPOINT_URL,
Expand All @@ -19,12 +17,8 @@
SFN_IAM_ROLE,
)
from metaflow.parameters import deploy_time_eval
from metaflow.plugins.aws.batch.batch_decorator import BatchDecorator
from metaflow.plugins.resources_decorator import ResourcesDecorator
from metaflow.plugins.retry_decorator import RetryDecorator
from metaflow.util import compress_list, dict_to_cli_options, to_pascalcase
from metaflow.util import dict_to_cli_options, to_pascalcase

from ..aws_utils import compute_resource_attributes
from ..batch.batch import Batch
from .event_bridge_client import EventBridgeClient
from .step_functions_client import StepFunctionsClient
Expand Down Expand Up @@ -664,11 +658,6 @@ def _batch(self, node):
batch_deco = [deco for deco in node.decorators if deco.name == "batch"][0]
resources = {}
resources.update(batch_deco.attributes)
resources.update(
compute_resource_attributes(
node.decorators, batch_deco, batch_deco.resource_defaults
)
)
Comment on lines -667 to -671
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

did not seem necessary as the same computation is happening inside the batch decorator as well.

# Resolve retry strategy.
user_code_retries, total_retries = self._get_retries(node)

Expand Down