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

connor-mccarthy · 2022-03-18T18:24:23Z

This is a follow-up PR addressing a comment from @chensun on #7428.

Description of changes

migrate modules to >= python 3.7
update test images to greatest supported python version
remove unused import

Checklist:

The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.

google-oss-prow · 2022-03-18T18:24:25Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

connor-mccarthy · 2022-03-18T18:24:26Z

/test all

connor-mccarthy · 2022-03-18T18:39:16Z

/retest

connor-mccarthy · 2022-03-18T19:43:23Z

/test all

chensun

Title is a bit misleading, as we dropped Python 3.6 support via #7303.
This is more of some dead code cleanup, I think. Maybe reframe the title a bit?

chensun · 2022-03-18T20:47:25Z

sdk/python/kfp/compiler/compiler_test.py

@@ -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


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

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

chensun · 2022-03-18T20:49:32Z

sdk/python/kfp/components/structures.py

@@ -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


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

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

Updated: I reverted from Dict to OrderedDict in b72d22f.

Please let me know if this is not desired.

OrderedDict SGTM, thank you!

connor-mccarthy · 2022-03-18T21:20:07Z

Title is a bit misleading, as we dropped Python 3.6 support via #7303. This is more of some dead code cleanup, I think. Maybe reframe the title a bit?

Thanks @chensun. Updated.

sdk/python/kfp/compiler_cli_tests/test_data/pipeline_with_task_final_status_yaml.json

connor-mccarthy · 2022-03-21T16:31:30Z

/test kubeflow-pipelines-samples-v2

chensun · 2022-03-21T16:49:58Z

/lgtm

Thanks!

ji-yaqi

/lgtm
/approve

Thank you!

ji-yaqi · 2022-03-21T21:42:23Z

sdk/python/kfp/components/structures.py

@@ -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


OrderedDict SGTM, thank you!

google-oss-prow · 2022-03-21T21:43:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ji-yaqi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~sdk/OWNERS~~ [ji-yaqi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* migrate modules to >= python 3.7 * update test images to greatest supported python version * remove unused import * update test data * clean comment * undo 3.7 image change * revert 3.9 -> 3.7 * revert OrderedDict to Dict

connor-mccarthy added 3 commits March 18, 2022 12:23

migrate modules to >= python 3.7

c86ddf7

update test images to greatest supported python version

e7ccf28

remove unused import

567e067

connor-mccarthy requested review from chensun and ji-yaqi March 18, 2022 18:24

google-oss-prow bot added the do-not-merge/work-in-progress label Mar 18, 2022

google-oss-prow bot added the size/M label Mar 18, 2022

update test data

cbbf5d8

clean comment

7526829

connor-mccarthy marked this pull request as ready for review March 18, 2022 20:28

google-oss-prow bot removed the do-not-merge/work-in-progress label Mar 18, 2022

chensun reviewed Mar 18, 2022

View reviewed changes

connor-mccarthy changed the title ~~chore(sdk): remove python 3.6 support~~ chore(sdk): remove python 3.6 dead code Mar 18, 2022

undo 3.7 image change

6e8e1ad

chensun reviewed Mar 18, 2022

View reviewed changes

sdk/python/kfp/compiler_cli_tests/test_data/pipeline_with_task_final_status_yaml.json Outdated Show resolved Hide resolved

connor-mccarthy force-pushed the drop-3.6-support branch from 53413dd to f8be5fd Compare March 18, 2022 23:52

revert 3.9 -> 3.7

2838a96

connor-mccarthy force-pushed the drop-3.6-support branch from f8be5fd to 2838a96 Compare March 18, 2022 23:53

revert OrderedDict to Dict

b72d22f

google-oss-prow bot assigned chensun Mar 21, 2022

google-oss-prow bot added the lgtm label Mar 21, 2022

connor-mccarthy requested a review from chensun March 21, 2022 16:54

ji-yaqi reviewed Mar 21, 2022

View reviewed changes

google-oss-prow bot assigned ji-yaqi Mar 21, 2022

google-oss-prow bot added the approved label Mar 21, 2022

google-oss-prow bot merged commit 41caca8 into kubeflow:master Mar 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

connor-mccarthy commented Mar 18, 2022 •

edited

Loading

google-oss-prow bot commented Mar 18, 2022

connor-mccarthy commented Mar 18, 2022

connor-mccarthy commented Mar 18, 2022

connor-mccarthy commented Mar 18, 2022

chensun left a comment

chensun Mar 18, 2022

connor-mccarthy Mar 18, 2022 •

edited

Loading

chensun Mar 18, 2022

connor-mccarthy Mar 18, 2022

connor-mccarthy Mar 18, 2022

ji-yaqi Mar 21, 2022

connor-mccarthy commented Mar 18, 2022

connor-mccarthy commented Mar 21, 2022

chensun commented Mar 21, 2022

ji-yaqi left a comment

ji-yaqi Mar 21, 2022

google-oss-prow bot commented Mar 21, 2022

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

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

Conversation

connor-mccarthy commented Mar 18, 2022 • edited Loading

google-oss-prow bot commented Mar 18, 2022

connor-mccarthy commented Mar 18, 2022

connor-mccarthy commented Mar 18, 2022

connor-mccarthy commented Mar 18, 2022

chensun left a comment

Choose a reason for hiding this comment

chensun Mar 18, 2022

Choose a reason for hiding this comment

connor-mccarthy Mar 18, 2022 • edited Loading

Choose a reason for hiding this comment

chensun Mar 18, 2022

Choose a reason for hiding this comment

connor-mccarthy Mar 18, 2022

Choose a reason for hiding this comment

connor-mccarthy Mar 18, 2022

Choose a reason for hiding this comment

ji-yaqi Mar 21, 2022

Choose a reason for hiding this comment

connor-mccarthy commented Mar 18, 2022

connor-mccarthy commented Mar 21, 2022

chensun commented Mar 21, 2022

ji-yaqi left a comment

Choose a reason for hiding this comment

ji-yaqi Mar 21, 2022

Choose a reason for hiding this comment

google-oss-prow bot commented Mar 21, 2022

connor-mccarthy commented Mar 18, 2022 •

edited

Loading

connor-mccarthy Mar 18, 2022 •

edited

Loading