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 5 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
3 changes: 1 addition & 2 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 @@ -15,7 +15,6 @@
import json
import os
import shutil
import sys
import tempfile
import unittest

Expand Down Expand Up @@ -553,7 +552,7 @@ def test_use_task_final_status_in_non_exit_op_yaml(self):
- {name: message, type: PipelineTaskFinalStatus}
implementation:
container:
image: python:3.7
image: python:3.9
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to change this. Our default image for lightweight component is still Python:3.7

Copy link
Member Author

@connor-mccarthy connor-mccarthy Mar 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you. I misunderstood this template definition as having a functional effect for our tests.

command:
- echo
- {inputValue: message}
Expand Down
4 changes: 2 additions & 2 deletions sdk/python/kfp/compiler_cli_tests/test_data/pipeline_with_task_final_status_yaml.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@
"pipeline status:",
"{{$.inputs.parameters['status']}}"
],
"image": "python:3.7"
"image": "python:3.9"
connor-mccarthy marked this conversation as resolved.
Show resolved Hide resolved
}
},
"exec-print-op": {
Expand All @@ -75,7 +75,7 @@
"echo",
"{{$.inputs.parameters['message']}}"
],
"image": "python:3.7"
"image": "python:3.9"
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions sdk/python/kfp/compiler_cli_tests/test_data/pipeline_with_task_final_status_yaml.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
- {name: status, type: PipelineTaskFinalStatus}
implementation:
container:
image: python:3.7
image: python:3.9
command:
- echo
- "user input:"
Expand All @@ -40,7 +40,7 @@
- {name: message, type: String}
implementation:
container:
image: python:3.7
image: python:3.9
command:
- echo
- {inputValue: message}
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
8 changes: 3 additions & 5 deletions sdk/python/kfp/components/structures.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
import dataclasses
import itertools
import json
from typing import Any, Dict, Mapping, Optional, Sequence, Union
from typing import Any, Dict, Mapping, Optional, OrderedDict, Sequence, Union

import pydantic
import yaml
Expand Down Expand Up @@ -305,12 +305,10 @@ 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
outputs: Optional[Dict[str, OutputSpec]] = None
inputs: Optional[OrderedDict[str, InputSpec]] = None
outputs: Optional[OrderedDict[str, OutputSpec]] = None
implementation: Implementation

@pydantic.validator('inputs', 'outputs', allow_reuse=True)
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