-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat: Support Optional
samplesheet parameters
#475
Open
msto
wants to merge
9
commits into
latchbio:main
Choose a base branch
from
msto:ms_optional-samplesheet-param
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
f60f342
refactor: extract _is_valid_samplesheet_parameter_type
msto 939d3e1
feat: Support Optional samplesheet types
msto 21590f9
chore: clean-up
msto f506b5b
test: Test PEP 604 types
msto 03c44f5
test: add PEP 585 and 604 samplesheet types
msto ede1b0b
test: cover non-samplesheet types
msto 40e01e4
fix: PR requests
msto 4f4ae28
fix: PR updates
msto 8c50966
refactor: import _GenericAlias from typing
msto File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Empty file.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,154 @@ | ||
import inspect | ||
import sys | ||
from dataclasses import dataclass | ||
from typing import List | ||
from typing import Any | ||
from typing import Collection, Iterable, Optional, Union, Mapping, Dict, Set, Tuple | ||
|
||
import pytest | ||
|
||
from latch.resources.workflow import _is_list_of_dataclasses_type | ||
from latch.resources.workflow import _is_valid_samplesheet_parameter_type | ||
from latch.resources.workflow import _is_optional_type | ||
from latch.resources.workflow import _is_type_annotation | ||
from latch.resources.workflow import _unpack_optional_type | ||
from latch.resources.workflow import TypeAnnotation | ||
|
||
PRIMITIVE_TYPES: List[type] = [int, float, bool, str] | ||
COLLECTION_TYPES: List[TypeAnnotation] = [List[int], Dict[str, int], Set[int], Tuple[int], Mapping[str, int], Iterable[int], Collection[int]] | ||
|
||
if sys.version_info >= (3, 10): | ||
COLLECTION_TYPES += [list[int], dict[str, int], set[int], tuple[int]] | ||
|
||
OPTIONAL_TYPES: List[TypeAnnotation] = [Optional[T] for T in (PRIMITIVE_TYPES + COLLECTION_TYPES)] | ||
OPTIONAL_TYPES += [Union[T, None] for T in (PRIMITIVE_TYPES + COLLECTION_TYPES)] | ||
|
||
if sys.version_info >= (3, 10): | ||
OPTIONAL_TYPES += [T | None for T in (PRIMITIVE_TYPES + COLLECTION_TYPES)] | ||
|
||
|
||
@dataclass | ||
class FakeDataclass: | ||
"""A dataclass for testing.""" | ||
foo: str | ||
bar: int | ||
|
||
|
||
# Enumerate the possible ways to declare a list or optional list of dataclasses | ||
SAMPLESHEET_TYPES: List[TypeAnnotation] = [ | ||
List[FakeDataclass], | ||
Optional[List[FakeDataclass]], | ||
Union[List[FakeDataclass], None], | ||
] | ||
|
||
if sys.version_info >= (3, 10): | ||
SAMPLESHEET_TYPES += [ | ||
list[FakeDataclass], | ||
Optional[list[FakeDataclass]], | ||
Union[list[FakeDataclass], None], | ||
list[FakeDataclass] | None, | ||
List[FakeDataclass] | None, | ||
] | ||
|
||
BAD_SAMPLESHEET_TYPES: List[TypeAnnotation] = PRIMITIVE_TYPES + COLLECTION_TYPES + OPTIONAL_TYPES | ||
BAD_SAMPLESHEET_TYPES += [ | ||
Union[List[FakeDataclass], int], | ||
Union[Optional[List[FakeDataclass]], int], | ||
Optional[Union[List[FakeDataclass], int]], | ||
Union[List[FakeDataclass], Optional[int]], | ||
Union[Optional[int], List[FakeDataclass]], | ||
List[Union[FakeDataclass, int]], | ||
List[Optional[FakeDataclass]], | ||
] | ||
|
||
|
||
@pytest.mark.parametrize("dtype", SAMPLESHEET_TYPES) | ||
def test_is_valid_samplesheet_parameter_type(dtype: TypeAnnotation) -> None: | ||
""" | ||
`_is_valid_samplesheet_parameter_type` should accept a type that is a list of dataclasses, or an | ||
`Optional` list of dataclasses. | ||
""" | ||
assert _is_valid_samplesheet_parameter_type(dtype) is True | ||
|
||
|
||
@pytest.mark.parametrize("dtype", BAD_SAMPLESHEET_TYPES) | ||
def test_is_valid_samplesheet_parameter_type_rejects_invalid_types(dtype: TypeAnnotation) -> None: | ||
""" | ||
`_is_valid_samplesheet_parameter_type` should reject any other type. | ||
""" | ||
assert _is_valid_samplesheet_parameter_type(dtype) is False | ||
|
||
|
||
def test_is_list_of_dataclasses_type() -> None: | ||
""" | ||
`_is_list_of_dataclasses_type` should accept a type that is a list of dataclasses. | ||
""" | ||
assert _is_list_of_dataclasses_type(List[FakeDataclass]) is True | ||
|
||
|
||
@pytest.mark.parametrize("bad_type", [ | ||
str, # Not a list | ||
int, # Not a list | ||
List[str], # Not a list of dataclasses | ||
List[int], # Not a list of dataclasses | ||
FakeDataclass, # Not a list | ||
]) | ||
def test_is_list_of_dataclasses_type_rejects_bad_type(bad_type: type) -> None: | ||
""" | ||
`_is_list_of_dataclasses_type` should reject anything else. | ||
""" | ||
assert _is_list_of_dataclasses_type(bad_type) is False | ||
|
||
|
||
def test_is_list_of_dataclasses_type_raises_if_not_a_type() -> None: | ||
""" | ||
`is_list_of_dataclasses_type` should raise a `TypeError` if the input is not a type. | ||
""" | ||
with pytest.raises(TypeError): | ||
_is_list_of_dataclasses_type([FakeDataclass("hello", 1)]) | ||
|
||
|
||
@pytest.mark.parametrize("dtype", OPTIONAL_TYPES) | ||
def test_is_optional_type(dtype: TypeAnnotation) -> None: | ||
"""`_is_optional_type` should return True for `Optional[T]` types.""" | ||
assert _is_optional_type(dtype) is True | ||
|
||
|
||
@pytest.mark.parametrize("dtype", PRIMITIVE_TYPES + COLLECTION_TYPES) | ||
def test_is_optional_type_returns_false_if_not_optional(dtype: TypeAnnotation) -> None: | ||
"""`_is_optional_type` should return False for non-Optional types.""" | ||
assert _is_optional_type(dtype) is False | ||
|
||
|
||
@pytest.mark.parametrize("dtype", PRIMITIVE_TYPES + COLLECTION_TYPES) | ||
def test_unpack_optional_type(dtype: TypeAnnotation) -> None: | ||
"""`_unpack_optional_type()` should return the base type of `Optional[T]` types.""" | ||
assert _unpack_optional_type(Optional[dtype]) is dtype | ||
assert _unpack_optional_type(Union[dtype, None]) is dtype | ||
if sys.version_info >= (3, 10): | ||
assert _unpack_optional_type(dtype | None) is dtype | ||
|
||
|
||
|
||
@pytest.mark.parametrize("annotation", PRIMITIVE_TYPES + COLLECTION_TYPES + OPTIONAL_TYPES) | ||
def test_is_type_annotation(annotation: TypeAnnotation) -> None: | ||
""" | ||
`_is_type_annotation()` should return True for any valid type annotation. | ||
""" | ||
assert _is_type_annotation(annotation) is True | ||
|
||
|
||
def test_is_type_annotation_returns_false_if_empty() -> None: | ||
""" | ||
`_is_type_annotation()` should only return False if the annotation is `Parameter.empty`. | ||
""" | ||
assert _is_type_annotation(inspect.Parameter.empty) is False | ||
|
||
|
||
@pytest.mark.parametrize("bad_annotation", [1, "abc", [1, 2], {"foo": 1}, FakeDataclass("hello", 1)]) | ||
def test_is_type_annotation_raises_if_annotation_is_not_a_type(bad_annotation: Any) -> None: | ||
""" | ||
`_is_type_annotation()` should raise `TypeError` for any non-type object. | ||
""" | ||
with pytest.raises(TypeError): | ||
_is_type_annotation(bad_annotation) |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
this is the wrong place for this check, i cant remember off the top of my head if we throw a semantic error already for not having a type annotation but we should do this outside of a samplesheet specific check (itll always be an error regardless of whether or not the specific param is a samplesheet)
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.
Given that the project does not enforce any type checking, I would prefer to include runtime checks on all argument types.
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.
(I'm happy to make this raise a
TypeError
instead, though in context I thought returningFalse
was sensible.)