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

chore(sdk): remove python 3.6 dead code #7440

Merged
merged 8 commits into from
Mar 21, 2022
Merged

chore(sdk): remove python 3.6 dead code #7440

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c86ddf7
migrate modules to >= python 3.7
connor-mccarthy Mar 18, 2022
e7ccf28
update test images to greatest supported python version
connor-mccarthy Mar 18, 2022
567e067
remove unused import
connor-mccarthy Mar 18, 2022
cbbf5d8
update test data
connor-mccarthy Mar 18, 2022
7526829
clean comment
connor-mccarthy Mar 18, 2022
6e8e1ad
undo 3.7 image change
connor-mccarthy Mar 18, 2022
2838a96
revert 3.9 -> 3.7
connor-mccarthy Mar 18, 2022
b72d22f
revert OrderedDict to Dict
connor-mccarthy Mar 18, 2022
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
5 changes: 2 additions & 3 deletions sdk/python/kfp/compiler/compiler_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -572,9 +572,8 @@ def my_pipeline(text: bool):

# pylint: disable=import-outside-toplevel,unused-import,import-error,redefined-outer-name,reimported
class V2NamespaceAliasTest(unittest.TestCase):
"""Test that imports of both modules and objects are aliased
(e.g. all import path variants work).
"""
"""Test that imports of both modules and objects are aliased (e.g. all
import path variants work)."""

# Note: The DeprecationWarning is only raised on the first import where
# the kfp.v2 module is loaded. Due to the way we run tests in CI/CD, we cannot ensure that the kfp.v2 module will first be loaded in these tests,
Expand Down
8 changes: 1 addition & 7 deletions sdk/python/kfp/components/component_factory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,15 +127,9 @@ def _annotation_to_type_struct(annotation):
else:
type_name = str(annotation.__name__)

# TODO: remove the hack once drop Python 3.6 support.
# In Python 3.6+, typing.Dict, typing.List are not instance of type.
if type_annotations.get_short_type_name(
str(annotation)) in ['List', 'Dict']:
type_name = str(annotation)

elif hasattr(
annotation, '__forward_arg__'
): # Handling typing.ForwardRef('Type_name') (the name was _ForwardRef in python 3.5-3.6)
): # Handling typing.ForwardRef('Type_name')
type_name = str(annotation.__forward_arg__)
else:
type_name = str(annotation)
Expand Down
2 changes: 0 additions & 2 deletions sdk/python/kfp/components/structures.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,8 +305,6 @@ class ComponentSpec(BaseModel):
implementation: The implementation of the component. Either an executor
(container, importer) or a DAG consists of other components.
"""
# TODO(ji-yaqi): Update to OrderedDict for inputs and outputs once we drop
# Python 3.6 support
Copy link
Member

Choose a reason for hiding this comment

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

I forget the context on this TODO. Doesn't Dict guarantee order already?
Do check with @ji-yaqi though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, @chensun. I think you're right... explanation:

Dicts are officially ordered in >= Python 3.7. This is a nuanced change: Python dicts are ordered, but are not OrderedDicts. We accept dict (e.g., dictionaries without the special OrderedDict methods), so it seems like we should not needlessly narrow this type from the superclass dict to the subclass typing.OrderedDict.

In code:

import typing
import collections

d = {"a": True, "b": False}
od = collections.OrderedDict({"a": True, "b": False})

assert isinstance(od, dict) # type-checking would pass with the looser type
assert not isinstance(d, typing.OrderedDict) # type-checking would fail with the narrower type

@ji-yaqi, do you agree? If so, I will revert.

Further reading: Choosing Between OrderedDict and dict

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated: I reverted from Dict to OrderedDict in b72d22f.

Please let me know if this is not desired.

Copy link
Contributor

Choose a reason for hiding this comment

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

OrderedDict SGTM, thank you!

name: str
description: Optional[str] = None
inputs: Optional[Dict[str, InputSpec]] = None
Expand Down
24 changes: 8 additions & 16 deletions sdk/python/kfp/components/v1_modelbase.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,7 @@ def verify_object_against_type(x: Any, typ: Type[T]) -> T:
raise TypeError(
'Error: None object is incompatible with type {}'.format(typ))

#assert isinstance(x, typ.__origin__)
generic_type = typ.__origin__ or getattr(
typ, '__extra__', None
) #In python <3.7 typing.List.__origin__ == None; Python 3.7 has working __origin__, but no __extra__ TODO: Remove the __extra__ once we move to Python 3.7
generic_type = typ.__origin__
if generic_type in [
list, List, abc.Sequence, abc.MutableSequence, Sequence,
MutableSequence
Expand All @@ -79,7 +76,7 @@ def verify_object_against_type(x: Any, typ: Type[T]) -> T:
raise TypeError(
'Error: Object "{}" is incompatible with type "{}"'.format(
x, typ))
# In Python <3.7 Mapping.__args__ is None.

# In Python 3.9 typ.__args__ does not exist when the generic type does not have subscripts
type_args = typ.__args__ if getattr(
typ, '__args__', None) is not None else (Any, Any)
Expand All @@ -96,7 +93,7 @@ def verify_object_against_type(x: Any, typ: Type[T]) -> T:
raise TypeError(
'Error: Object "{}" is incompatible with type "{}"'.format(
x, typ))
# In Python <3.7 Mapping.__args__ is None.

# In Python 3.9 typ.__args__ does not exist when the generic type does not have subscripts
type_args = typ.__args__ if getattr(
typ, '__args__', None) is not None else (Any, Any)
Expand Down Expand Up @@ -157,9 +154,7 @@ def parse_object_from_struct_based_on_type(struct: Any, typ: Type[T]) -> T:
possible_types = list(getattr(typ, '__args__', [Any]))
#if type(None) in possible_types and struct is None: #Shortcut for Optional[] tests. Can be removed, but the exceptions will be more noisy.
# return None
#Hack for Python <3.7 which for some reason "simplifies" Union[bool, int, ...] to just Union[int, ...]
if int in possible_types:
possible_types = possible_types + [bool]

for possible_type in possible_types:
try:
obj = parse_object_from_struct_based_on_type(
Expand Down Expand Up @@ -194,10 +189,7 @@ def parse_object_from_struct_based_on_type(struct: Any, typ: Type[T]) -> T:
'Error: None structure is incompatible with type {}'.format(
typ))

#assert isinstance(x, typ.__origin__)
generic_type = typ.__origin__ or getattr(
typ, '__extra__', None
) #In python <3.7 typing.List.__origin__ == None; Python 3.7 has working __origin__, but no __extra__ TODO: Remove the __extra__ once we move to Python 3.7
generic_type = typ.__origin__
if generic_type in [
list, List, abc.Sequence, abc.MutableSequence, Sequence,
MutableSequence
Expand All @@ -206,7 +198,7 @@ def parse_object_from_struct_based_on_type(struct: Any, typ: Type[T]) -> T:
raise TypeError(
'Error: Structure "{}" is incompatible with type "{}" - it does not have list type.'
.format(struct, typ))
# In Python <3.7 Mapping.__args__ is None.

# In Python 3.9 typ.__args__ does not exist when the generic type does not have subscripts
type_args = typ.__args__ if getattr(
typ, '__args__', None) is not None else (Any, Any)
Expand All @@ -219,12 +211,12 @@ def parse_object_from_struct_based_on_type(struct: Any, typ: Type[T]) -> T:
elif generic_type in [
dict, Dict, abc.Mapping, abc.MutableMapping, Mapping,
MutableMapping, OrderedDict
]: #in Python <3.7 there is a difference between abc.Mapping and typing.Mapping
]:
if not isinstance(struct, generic_type):
raise TypeError(
'Error: Structure "{}" is incompatible with type "{}" - it does not have dict type.'
.format(struct, typ))
# In Python <3.7 Mapping.__args__ is None.

# In Python 3.9 typ.__args__ does not exist when the generic type does not have subscripts
type_args = typ.__args__ if getattr(
typ, '__args__', None) is not None else (Any, Any)
Expand Down