fix(sdk): Allow non-pythonic names for graph components' task's outputs. Fixes #4514.

[Udiknedormin]

Pythonic-to-original output name mapping got a default value equal to the output's name. Solves #4514, this is the suggested solution described there. Also adds tests to check if the problem was resolved.

It seems it could be cherry-picked in the current release branch.

[kubeflow-bot]

This change is 

[k8s-ci-robot]

Hi @Udiknedormin. 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.

[Tomcli]

/ok-to-test

[Tomcli]

/test kubeflow-pipelines-tfx-python36

[Tomcli]

It looks like the TFX test failed due to an older version of protobuf.
https://stackoverflow.com/questions/61922334/how-to-solve-attributeerror-module-google-protobuf-descriptor-has-no-attribu

[numerology]

Sent #4516 to fix.

Sorry for the inconvenience.

[Tomcli]

Thanks @numerology
@Udiknedormin can you rebase the PR once @numerology merged the fix? Thanks.

[Ark-kun]

I think this can be simpler:

Suggested change

	task_outputs_with_original_names = {
	# component_bridge generates outputs under both pythonic and original name,
	# so half of them are absent from pythonic_output_name_to_original
	pythonic_output_name_to_original.get(pythonic_output_name, pythonic_output_name): output_value
	for pythonic_output_name, output_value in task_outputs_with_pythonic_names.items()
	}
	task_outputs_with_original_names = {output.name: task_obj.outputs[output.name] for output in task_component_spec.outputs or []}

output_name_to_pythonic, pythonic_output_name_to_original and task_outputs_with_pythonic_names are not needed then.

[Udiknedormin]

@Ark-kun
Was it ever a part of the contract for _container_task_constructor? It's not backwards-compatible, but it certainly would simplify things.

I wasn't sure about it, considering that _container_task_constructor can be overwritten and I only know of two overwrites:

• _components._create_task_spec_from_component_and_arguments --- which uses original outputs' names, as seen in TaskSpec._init_outputs
• _component_bridghe._create_container_op_from_component_and_arguments --- which uses both, as shown in the issue

[Udiknedormin]

Would the following be ok?

KT = TypeVar('KT') # could be imported from typing, of course
VT = TypeVar('VT')
def get_maybe_remap(m: Mapping[KT, VT], key: KT,
                    key_mapper: Callable[[KT], KT]) -> VT:
    if key in m:
        return m[key]
    else:
        return m[key_mapper(key)]
(...)

task_outputs_with_original_names = {
    output.name: get_maybe_pythonic(task_obj.outputs, output.name,
                                    output_name_to_pythonic.__getitem__)
    for output in task_component_spec.outputs or []
}

It would remove the need for pythonic_output_name_to_original (i.e. reverse-mapping of output_name_to_pythonic) and task_outputs_with_pythonic_names, leaving just output_name_to_pythonic "just in case", for backward-compatibility. It's also reusable.

[Ark-kun]

Was it ever a part of the contract for _container_task_constructor? It's not backwards-compatible, but it certainly would simplify things.

I think this should be the contract (original output names should be the keys in the task.outputs dictionary).
I think at his point it's still OK it make this change. The contract is private and I do not think anyone else has implemented the contact yet (also we're making a significantly more drastic change to it very soon).
We can always restore the behavior is we encounter unfixable case in the future.

[Udiknedormin]

@Ark-kun Ok, if it can be changed, then I'll simplify it and add the explicit contract.

[Ark-kun]

/retest

[Udiknedormin]

@Tomcli

can you rebase the PR once @numerology merged the fix?

Done.

[Ark-kun]

/retest

[Udiknedormin]

@Ark-kun I added the contract and Protocol for both the ResolvedTask and ContainerTaskConstructor. I think it should be clear enough now. As a bonus, it should provide a little better user experience for type-checker users.

[Ark-kun]

I added the contract and Protocol for both the ResolvedTask and ContainerTaskConstructor. I think it should be clear enough now.

I really appreciate you Protocol proposal, but is it possible to stage this work into two PRs? To first just fix the bug and add the tests and then do the refactoring in another PR.
Protocols are interesting, but we need to get them right. For example the Task object interface is only expected to have outputs map. There are no more expectation at this moment.
There also is an ongoing refactoring in this place which will change the ContainerTaskConstructor protocol.

[Udiknedormin]

@Ark-kun Fine by me. Reverted to the minimal version.

[Ark-kun]

/lgtm
/approve

JFYI, the refactorings that I've mentioned are #3447, #3448 . I'll keep the idea of protocols in mind. Your previous code is available here: bef604c

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ark-kun

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 [Ark-kun]

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