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

Change supertype calculation when combining scatters #5101

Merged
merged 2 commits into from
Sep 24, 2024
Merged

Change supertype calculation when combining scatters #5101

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bc5e2ee
Change supertype calculation when combining scatters
stxue1 Sep 19, 2024
13ccbaf
Simplify if else branch
stxue1 Sep 23, 2024
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
45 changes: 18 additions & 27 deletions src/toil/wdl/wdltoil.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,36 +505,27 @@ def log_bindings(log_function: Callable[..., None], message: str, all_bindings:
elif isinstance(bindings, Promise):
log_function("<Unfulfilled promise for bindings>")

def get_supertype(types: Sequence[Optional[WDL.Type.Base]]) -> WDL.Type.Base:
def get_supertype(types: Sequence[WDL.Type.Base]) -> WDL.Type.Base:
"""
Get the supertype that can hold values of all the given types.
"""

if None in types:
# Need to allow optional values
if len(types) == 1:
# Only None is here
return WDL.Type.Any(optional=True)
if len(types) == 2:
# None and something else
for item in types:
if item is not None:
# Return the type that's actually there, but make optional if not already.
return item.copy(optional=True)
raise RuntimeError("Expected non-None in types could not be found")
else:
# Multiple types, and some nulls, so we need an optional Any.
return WDL.Type.Any(optional=True)
else:
if len(types) == 1:
# Only one type. It isn't None.
the_type = types[0]
if the_type is None:
raise RuntimeError("The supertype cannot be None.")
return the_type
supertype = None
optional = False
for typ in types:
if isinstance(typ, WDL.Type.Any):
# ignore an Any type, as we represent a bottom type as Any. See https://miniwdl.readthedocs.io/en/latest/WDL.html#WDL.Type.Any
# and https://github.com/openwdl/wdl/blob/e43e042104b728df1f1ad6e6145945d2b32331a6/SPEC.md?plain=1#L1484
optional = optional or typ.optional
else:
# Multiple types (or none). Assume Any
return WDL.Type.Any()
if supertype is None:
supertype = typ
optional = optional or typ.optional
else:
# We have conflicting types
raise RuntimeError(f"Cannot generate a supertype from conflicting types: {types}")
Copy link
Member

@DailyDreaming DailyDreaming Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This can be simplified... but I'm not sure that the logic still holds up? optional seems like it will be set to the previous typ in types, if True, and will remain True the entire loop thereafter.

Here's the simplification:

supertype = None
optional = False
for typ in types:
    if isinstance(typ, WDL.Type.Any):
        # ignore an Any type, as we represent a bottom type as Any. See https://miniwdl.readthedocs.io/en/latest/WDL.html#WDL.Type.Any
        # and https://github.com/openwdl/wdl/blob/e43e042104b728df1f1ad6e6145945d2b32331a6/SPEC.md?plain=1#L1484
        optional = optional or typ.optional
    else:
        if supertype is None:
            supertype = typ
            optional = optional or typ.optional
        else:
            # We have conflicting types
            raise RuntimeError(f"Cannot generate a supertype from conflicting types: {types}")

==>

supertype = None
optional = False
for typ in types:
    if isinstance(typ, WDL.Type.Any):
        # ignore an Any type, as we represent a bottom type as Any. See https://miniwdl.readthedocs.io/en/latest/WDL.html#WDL.Type.Any
        # and https://github.com/openwdl/wdl/blob/e43e042104b728df1f1ad6e6145945d2b32331a6/SPEC.md?plain=1#L1484
        optional = optional or typ.optional
    elif supertype is None:
        supertype = typ
        optional = optional or typ.optional
    else:
        # We have conflicting types
        raise RuntimeError(f"Cannot generate a supertype from conflicting types: {types}")

==>

supertype = None
optional = False
for typ in types:
    optional = optional or typ.optional
    if supertype is None and not isinstance(typ, WDL.Type.Any):
        supertype = typ
    elif not isinstance(typ, WDL.Type.Any):
        # We have conflicting types
        raise RuntimeError(f"Cannot generate a supertype from conflicting types: {types}")

Is this still what it needs to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this should be functionally similar, I can adjust the comment to give some clarification about the Any type as that is pretty important to keep

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will actually break when the types list has multiple types in them, so I'll keep it as is

if supertype is None:
return WDL.Type.Any(null=optional) # optional flag isn't used in Any
return supertype.copy(optional=optional)


def for_each_node(root: WDL.Tree.WorkflowNode) -> Iterator[WDL.Tree.WorkflowNode]:
Expand Down Expand Up @@ -3227,7 +3218,7 @@ def run(self, file_store: AbstractFileStore) -> WDLBindings:
# Problem: the WDL type types are not hashable, so we need to do bad N^2 deduplication
observed_types = []
for env in new_bindings:
binding_type = env.resolve(name).type if env.has_binding(name) else None
binding_type = env.resolve(name).type if env.has_binding(name) else WDL.Type.Any()
if binding_type not in observed_types:
observed_types.append(binding_type)
# Get the supertype of those types
Expand Down
Loading