Pipelines: Constructing dsl.ContainerOp instance directly in user code doesn't always raise the warning.

[chensun]

What steps did you take:

Compile the following pipeline

import kfp


def _sample_component_op(
    name: str, run_id: str, sample_metric_value: float
) -> kfp.dsl.ContainerOp:
    arguments = [run_id, sample_metric_value]
    return kfp.dsl.ContainerOp(
        name=name,
        image='library/hello-world:latest',
        arguments=arguments,
    )


@kfp.dsl.pipeline(name='minimul')
def sample_pipeline(
    run_id: str = kfp.dsl.RUN_ID_PLACEHOLDER,
    sample_metric_value: float = 0.5,
):
    """Execute main procedure of the pipeline."""
    _sample_component_op("component", run_id, sample_metric_value)

What happened:

No warning raised on using ContainerOp directly.

What did you expect to happen:

Seeing the following warning:

/usr/local/lib/python3.7/dist-packages/kfp/dsl/_container_op.py:1039: FutureWarning: Please create reusable components instead of constructing ContainerOp instances directly. Reusable components are shareable, portable and have compatibility and support guarantees. Please see the documentation: https://www.kubeflow.org/docs/pipelines/sdk/component-development/#writing-your-component-definition-file The components can be created manually (or, in case of python, using kfp.components.create_component_from_func or func_to_container_op) and then loaded using kfp.components.load_component_from_file, load_component_from_uri or load_component_from_text: https://kubeflow-pipelines.readthedocs.io/en/stable/source/kfp.components.html#kfp.components.load_component_from_file
  category=FutureWarning,

KFP SDK version: 1.4.0

Anything else you would like to add:

For comparison, compiling the following example raises the warning as expected.

import kfp

@kfp.dsl.pipeline(name='hello-world-pipeline')
def sample_pipeline():
  kfp.dsl.ContainerOp(
      name='hello-world',
      image='library/hello-world:latest',
      arguments=[],
  )

/kind bug
/area sdk

[chensun]

/assign @chensun
/cc @Ark-kun

[Ark-kun]

Hmm.
Interesting. We had this issue before and @munagekar fixed it by changing the warning type from DeprecationWarning to FutureWarning. https://github.com/kubeflow/pipelines/pull/4658

It looks like that warning is still not show to the users even when we see it in tests.

[Ark-kun]

This is really weird. Unlike DeprecationWarning, FutureWarning should always be shown: https://docs.python.org/3/library/warnings.html#default-warning-filter

I can reproduce the behavior. I also see that when the warning is shown it's only shown once (in a Jupyter session).

[munagekar]

I also see that when the warning is shown it's only shown once (in a Jupyter session).

This is the usual behavior with python warnings. Warning in a loop get called once only.

This is really weird. Unlike DeprecationWarning, FutureWarning should always be shown

https://docs.python.org/3/library/warnings.html#default-warning-filter
This is correct, however this is not a python issue.

I have investigated this further, the reason the compiler doesn't produce the warning is because the code in the first case does not reach the warning. The attached screenshot explains it further.

https://github.com/kubeflow/pipelines/blob/a12e88d1da57b897a3a25b5c44540fc7d3c9a40e/sdk/python/kfp/dsl/_container_op.py#L1104-L1105

The core issue is that __eq__ in kfp.dsl._pipeline_param.PipelineParam is incorrectly returning NamedTuple. __eq__should return either True or False. A named tuple is assumed to be True.
https://github.com/kubeflow/pipelines/blob/67afca4938c35e6b8be29e61df3883af11554220/sdk/python/kfp/dsl/_pipeline_param.py#L227-L228

[AseiSugiyama]

Thank you for clear explanation. We are facing this issue and want to solve it.

Let me try to fix this issue, I'll create PR.

[AseiSugiyama]

I created PR https://github.com/kubeflow/pipelines/pull/5299

[Ark-kun]

The core issue is that __eq__ in kfp.dsl._pipeline_param.PipelineParam is incorrectly returning NamedTuple. __eq__should return either True or False. A named tuple is assumed to be True.

Oh my! Thank you for the great investigation.