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: apply only supported attributes from resources decorator in batch #1586

Merged
merged 2 commits into from
Oct 24, 2023
Merged

fix: apply only supported attributes from resources decorator in batch #1586

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eb1955d
filter attributes pulled from resource_decorator for batch to only th…
saikonen Oct 12, 2023
1395cb6
support all decorator attributes, not just resource_defaults
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
5 changes: 5 additions & 0 deletions metaflow/plugins/aws/aws_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,11 @@ def compute_resource_attributes(decos, compute_deco, resource_defaults):
# We use the non None value if there is only one or the larger value
# if they are both non None. Note this considers "" to be equivalent to
# the value zero.
#
# Skip attributes that are not supported by the decorator.
if k not in [*resource_defaults.keys(), *deco.attributes.keys()]:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this will be recomputed everytime. May be better to make it a set initially and then compare for inclusion. Here for every k, we will recompute the full list.

This also doesn't seem to do what the comment says. My understanding was that it would only let through attributes that are known to, for example, the batch decorator (that's the resource_defaults part) whereas here it lets through anything that is known to the batch_decorator OR the resources decorator which seems to not be exactly what we want (ie: no real different from what it currently is.

continue

if my_val is None and v is None:
continue
if my_val is not None and v is not None:
Expand Down