fix(sdk.v2): Component is invalid if `packages_to_install` empty and `install_kfp_package=False` #6527

judahrand · 2021-09-08T14:23:27Z

Description of your changes:

Add test for empty packages_to_install_command
Don't return pip install with no packages

If an empty list is provided no command should be returned to avoid a
broken command eg. pip install

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-cla · 2021-09-08T14:23:34Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

google-oss-robot · 2021-09-08T14:23:39Z

Hi @judahrand. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

judahrand · 2021-09-08T14:25:17Z

@googlebot I signed it!

chensun · 2021-09-09T01:23:58Z

sdk/python/kfp/components_tests/test_python_op.py

@@ -1158,6 +1158,12 @@ def test_packages_to_install_feature(self):
            self.helper_test_component_using_local_call(
                task_factory2, arguments={}, expected_output_values={})

+        task_factory3 = comp.func_to_container_op(
+            dummy_in_0_out_0, packages_to_install=[], install_kfp_package=False)


func_to_container_op doesn't have the install_kfp_package argument.
Besides, I don't think any test under this folder would cover your change under v2/components.

You're right, let me remove the commit which adds this test. Do you think this is worth adding a test for?
It certainly caused me problems...

chensun · 2021-09-09T02:48:36Z

sdk/python/kfp/v2/components/component_factory.py

@@ -35,7 +35,7 @@ def _python_function_name_to_component_name(name):
 def _get_packages_to_install_command(
        package_list: Optional[List[str]] = None) -> List[str]:
    result = []
-    if package_list is not None:


An empty list is a user input, while None is the default value by the system. I thinks it's fine to make this change, but arguably empty list is a user error we don't necessarily need to catch - user could also provide a list of invalid packages.

That's not true, see:

pipelines/sdk/python/kfp/v2/components/component_factory.py

Line 154 in d83933d

packages_to_install = packages_to_install or []

In the code the None is replaced with an empty list anyway. So leaving the default None will result in the empty list if install_kfp_package is False.

I see, maybe an alternative change it to move packages_to_install = packages_to_install or [] on line 154 to line 158:

pipelines/sdk/python/kfp/v2/components/component_factory.py

Lines 154 to 158 in 2f19a26

packages_to_install = packages_to_install or []

if install_kfp_package:

if kfp_package_path is None:

kfp_package_path = _get_default_kfp_package_path()

packages_to_install.append(kfp_package_path)

WDYT?

Personally I think I prefer the current change and would maybe also remove the Optional from _get_packages_to_install_command.

Imo _get_packages_to_install_command should have enough logic (and does) to identify when there are no packages (empty list). It accepting a None type too just muddies the water in my mind. I'm not sure I really understand the advantage of it taking None

It accepting a None type too just muddies the water in my mind. I'm not sure I really understand the advantage of it taking None

Please don't remove the Optional. It's not a good idea to use mutable objects as default arguments:
https://google.github.io/styleguide/pyguide.html#212-default-argument-values

I personally prefer the alternative change I mentioned above, in which way packages_to_install == None indicates system default, whereas packages_to_install == [] must be from user input.

That said, I'm fine with the current change as well.

@neuromage That's not what I was suggesting at all! I'm fully aware that mutable default args are a very bad idea.

My suggestion was simply to remove the Optional type hint on _get_packages_to_install_command to make it clear that a list is expected.

It's a private function (as much as such a thing exists in Python) which is only used internally so there's not really a good reason for the Optional in my mind. And it already doesn't have a default value.

It's a private function (as much as such a thing exists in Python) which is only used internally so there's not really a good reason for the Optional in my mind. And it already doesn't have a default value.

Optional[...] is just a shorthand for Union[None, ...], it has nothing to do with default value. With the alternative change I commented above, we would get None--the default value from the public method--passed all the way through to this point. So Optional would make sense.

neuromage · 2021-09-09T22:17:29Z

/ok-to-test

chensun

Hi @judahrand, can you please add an entry for this PR in the SDK release note: https://github.com/kubeflow/pipelines/blob/master/sdk/RELEASE.md

chensun · 2021-09-10T02:12:19Z

sdk/python/kfp/v2/components/component_factory.py

@@ -35,7 +35,7 @@ def _python_function_name_to_component_name(name):
 def _get_packages_to_install_command(
        package_list: Optional[List[str]] = None) -> List[str]:
    result = []
-    if package_list is not None:


I personally prefer the alternative change I mentioned above, in which way packages_to_install == None indicates system default, whereas packages_to_install == [] must be from user input.

That said, I'm fine with the current change as well.

If an empty list is provided no command should be returned to avoid a broken command eg. `pip install `

chensun

/lgtm
/approve

Thanks!

chensun · 2021-09-15T08:41:30Z

sdk/RELEASE.md

@@ -11,6 +11,7 @@
 ## Deprecations

 ## Bug Fixes and Other Changes
+* Fix component decorator could result in invalid component if `install_kfp_package=False`. [\#6527](https://github.com/kubeflow/pipelines/pull/6527))


nit: there's extra ) at the end. I'll fix it later when doing the release.

google-oss-robot · 2021-09-15T08:42:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chensun

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~~ [chensun]

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

chensun · 2021-09-15T09:23:27Z

/retest

chensun · 2021-09-15T23:40:09Z

/retest

google-oss-robot requested review from Ark-kun and chensun September 8, 2021 14:23

google-cla bot added the cla: no label Sep 8, 2021

google-oss-robot added size/XS needs-ok-to-test labels Sep 8, 2021

google-cla bot added cla: yes and removed cla: no labels Sep 8, 2021

judahrand changed the title ~~fix(sdk): Component is invalid if packages_to_install empty and install_kfp_package=False~~ fix(sdk.v2): Component is invalid if packages_to_install empty and install_kfp_package=False Sep 8, 2021

chensun reviewed Sep 9, 2021

View reviewed changes

judahrand force-pushed the bug/packages-to-install branch from d83933d to 0fe114a Compare September 9, 2021 22:12

google-oss-robot added ok-to-test and removed needs-ok-to-test labels Sep 9, 2021

chensun reviewed Sep 10, 2021

View reviewed changes

judahrand force-pushed the bug/packages-to-install branch from 490b4bf to 667393e Compare September 10, 2021 10:17

Don't return pip install with no packages

519751d

If an empty list is provided no command should be returned to avoid a broken command eg. `pip install `

judahrand force-pushed the bug/packages-to-install branch from 667393e to 519751d Compare September 14, 2021 08:05

chensun reviewed Sep 15, 2021

View reviewed changes

google-oss-robot assigned chensun Sep 15, 2021

google-oss-robot added the lgtm label Sep 15, 2021

google-oss-robot added the approved label Sep 15, 2021

google-oss-robot merged commit a52ac6d into kubeflow:master Sep 16, 2021

judahrand mentioned this pull request Sep 16, 2021

Add judahrand to member list kubeflow/internal-acls#492

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sdk.v2): Component is invalid if `packages_to_install` empty and `install_kfp_package=False` #6527

fix(sdk.v2): Component is invalid if `packages_to_install` empty and `install_kfp_package=False` #6527

judahrand commented Sep 8, 2021 •

edited

Loading

google-cla bot commented Sep 8, 2021

google-oss-robot commented Sep 8, 2021

judahrand commented Sep 8, 2021

chensun Sep 9, 2021

judahrand Sep 9, 2021

chensun Sep 9, 2021

judahrand Sep 9, 2021 •

edited

Loading

chensun Sep 9, 2021

judahrand Sep 9, 2021 •

edited

Loading

neuromage Sep 9, 2021

chensun Sep 10, 2021

judahrand Sep 10, 2021 •

edited

Loading

chensun Sep 15, 2021

neuromage commented Sep 9, 2021

chensun left a comment

chensun Sep 10, 2021

chensun left a comment

chensun Sep 15, 2021

google-oss-robot commented Sep 15, 2021

chensun commented Sep 15, 2021

chensun commented Sep 15, 2021

	packages_to_install = packages_to_install or []
	if install_kfp_package:
	if kfp_package_path is None:
	kfp_package_path = _get_default_kfp_package_path()
	packages_to_install.append(kfp_package_path)

fix(sdk.v2): Component is invalid if packages_to_install empty and install_kfp_package=False #6527

fix(sdk.v2): Component is invalid if packages_to_install empty and install_kfp_package=False #6527

Conversation

judahrand commented Sep 8, 2021 • edited Loading

google-cla bot commented Sep 8, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

google-oss-robot commented Sep 8, 2021

judahrand commented Sep 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

judahrand Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

judahrand Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

judahrand Sep 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neuromage commented Sep 9, 2021

chensun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chensun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-oss-robot commented Sep 15, 2021

chensun commented Sep 15, 2021

chensun commented Sep 15, 2021

fix(sdk.v2): Component is invalid if `packages_to_install` empty and `install_kfp_package=False` #6527

fix(sdk.v2): Component is invalid if `packages_to_install` empty and `install_kfp_package=False` #6527

judahrand commented Sep 8, 2021 •

edited

Loading

judahrand Sep 9, 2021 •

edited

Loading

judahrand Sep 9, 2021 •

edited

Loading

judahrand Sep 10, 2021 •

edited

Loading