SDK - Hiding Argo's workflow.uid placeholder behind DSL

[Ark-kun]

Fixes #1673

---

This change is 

[gaoning777]

you might as well add all the argo dynamic params in the dsl and create a submodule called e.g. dsl.param

[Ark-kun]

The other ones seems to already been hidden by DSL (e.g. {{inputs.*}}, {{tasks.*}} etc).
What placeholders do you have in mind?

P.S. I want the DSL placeholders to be portable, so that they can be supported outside Argo.

[IronPan]

can we just use {{workflow.uid}} as discussed offline? and backend can replace with the run id.

[Ark-kun]

can we just use {{workflow.uid}} as discussed offline? and backend can replace with the run id.

I do remember our discussion. Do you think something needs to be changed in this PR?

[IronPan]

Took a second look. The change LGTM

[IronPan]

/lgtm

[gaoning777]

Should we rename the task_id to component_id or something? task_id might sound a little misleading to some of the users.

[gaoning777]

/hold

[Ark-kun]

Should we rename the task_id to component_id or something? task_id might sound a little misleading to some of the users.

"task" is an instance of "component".
If you launch three trainers, the component id is the same, but the task ids are different.

[gaoning777]

understood. maybe not component_id. However, the task_id concept is the internal concept in the component module.
@hongye-sun WDYT?

[Ark-kun]

However, the task_id concept is the internal concept in the component module.

The task_id here is different from the internal task_id (which is the id of a task in a DAG). Maybe we should call it execution_id. WDYT?

[Ark-kun]

@hongye-sun WDYT?

[Ark-kun]

Gentle ping everyone.

[elikatsis]

Hello there!

Have you thought of exposing {{workflow.name}} or {{pod.name}} similarly?

We already use {{workflow.name}} in VolumeOps and VolumeSnapshotOps prepending the names with it. But it could be of great use for naming purposes when users define custom resources and create them through ResourceOps. (task_id may not comply with K8s naming restrictions, when it starts with a number).

We could add it in a following PR, but I thought it is a good opportunity to introduce them all together.

[Ark-kun]

Have you thought of exposing {{workflow.name}} or {{pod.name}} similarly?

The goal here is to expose unique IDs of the run and task execution objects in an orchestrator-agnostic portable way.
Some problems with {{workflow.name}}:

• It's not unique and can be reused by Kubernetes
• There already is a proper unique pipeline run ID which is {{workflow.uid}}
  Some problems with {{pod.name}}:
• It's not unique and can be reused by Kubernetes
• It's already part of a proper unique task execution ID which is {{workflow.uid}}-{{pod.name}}

Can you give us some user scenarios where the two proposed IDs are insufficient?

task_id may not comply with K8s naming restrictions, when it starts with a number

We should fix this problem. I propose prefixing the IDs with run- and execution-. WDYT?

[elikatsis]

You are right, uniqueness might be a problem later on (e.g. when GC is performed in following Argo releases). I feel it is hard (not very likely) to occur, but it is still there.

My concern is that {{workflow.uid}} is quite large in terms of length, {{pod.name}} too. That makes them kinda hard to use for resource naming purposes (if a resource has even stricter naming conventions [1]). Also, it makes it hard for users to identify resources related to some execution.

Can you think of any alternative covering the case and being somewhat human-readable?
I'm leaning towards exposing both {{workflow.*}} variables.

Moreover, TFX Taxi Cab sample just came to my mind.
This sample deploys a service (kubeflow deployer component) and then this component handles the name truncated down to 64 characters length (I think it is 64, otherwise it's something close to that).
And I have run into the following problem: if user passes a long tf_server_name (i.e. tf_server_name + ID from generateName > 64 chars), the step will fail because it won't be able to verify the creation of the service or something like that.
So, I think (without having it tested) that this PR might break it.

[1]: Quoting Kubernetes documentation

By convention, the names of Kubernetes resources should be up to maximum length of 253 characters [...] but certain resources have more specific restrictions.

[Ark-kun]

@elikatsis Your concerns are valid, but I think they're a bit more like a feature request.
The purpose of this PR is to replace the "magic strings" in samples with constants. So this is just refactoring that does not introduce any change in behavior.

Components that want to pass the IDs to some strictly-formatted fields should do sanitization themselves. E.g. if one place only accepts dashed values and another place wants values with underscores.

We can think about introducing a placeholder for shorter IDs, but it would be a new placeholder, since the behavior is different.

[Ark-kun]

Maybe we should call it execution_id. WDYT?

Renamed task_id_placeholder to execution_id_placeholder.
/hold cancel

[Ark-kun]

/retest

[Ark-kun]

/cc @numerology I've made the changes you proposed.

[numerology]

/lgtm

[Ark-kun]

/approve

[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:

• components/OWNERS [Ark-kun]
• samples/OWNERS [Ark-kun]
• 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