feat(frontend): Upgrade argo template on UI. Fix #5358 #5359
Conversation
| /** | ||
| * Raw contains raw artifact location details | ||
| */ | ||
| raw?: RawArtifact; |
There was a problem hiding this comment.
Are you sure we want to remove this? The KFPv1 compiler uses raw artifacts.
There was a problem hiding this comment.
Argo removed it, which is strange...
Looks like at least KFP UI is not using it.
There was a problem hiding this comment.
It seems to still exist in Argo:
https://github.com/argoproj/argo-workflows/blob/be44ce9ddce219026292317d42c977486f10c43f/pkg/apis/workflow/v1alpha1/workflow_types.go#L796
https://github.com/argoproj/argo-workflows/blob/be44ce9ddce219026292317d42c977486f10c43f/pkg/apis/workflow/v1alpha1/workflow_types.go#L915
This artifact type is used when there is a need for a constant artifact: when a constant value is passed to a component which consumes it as a file.
There was a problem hiding this comment.
Although Argo workflow proto has it, I am not sure how it is being used on UI. It might be that this is being used on the SDK side but not on UI, so Argo UI has removed it from Artifact interface: https://github.com/argoproj/argo-workflows/blob/a8e9348261c77cb3b13bef864520128279f2e6b8/ui/src/models/workflows.ts#L28
However, I tend to be conservative before further confirmation. So I will keep the rawArtifact here for now.
|
For confirmation, which version of argo did you upgrade to? I'd suggest using v2.12.X (latest) |
There was a problem hiding this comment.
For confirmation, which version of argo did you upgrade to? I'd suggest using v2.12.X (latest)
That is a very good point! Thank you Yuan! Now I use the correct version v2.12.0 from Argo: https://github.com/argoproj/argo-workflows/blob/a8e9348261c77cb3b13bef864520128279f2e6b8/ui/src/models/workflows.ts#L28
| /** | ||
| * Raw contains raw artifact location details | ||
| */ | ||
| raw?: RawArtifact; |
There was a problem hiding this comment.
Although Argo workflow proto has it, I am not sure how it is being used on UI. It might be that this is being used on the SDK side but not on UI, so Argo UI has removed it from Artifact interface: https://github.com/argoproj/argo-workflows/blob/a8e9348261c77cb3b13bef864520128279f2e6b8/ui/src/models/workflows.ts#L28
However, I tend to be conservative before further confirmation. So I will keep the rawArtifact here for now.
|
@zijianjoy Is it a typo? Latest argo is 2.12.10 |
|
/approve /Hold |
That is a good catch! Confirming the version we need to copy from by starting over the following steps:
I have made adjustment for matching this file. |
|
Note, another way to find the commit is by looking at commit tags, there will be a tag for each version. They are the canonical source of truth for versions |
|
/lgtm Thank you for the upgrade! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Bobgy 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:
Approvers can indicate their approval by writing |
Thank you Yuan! Yes I confirm the tag |
|
/unhold |
Description of your changes:
Applied the change from Argo model to KFP UI third_party dependency. It is because we have upgraded to newer version of Argo and need to accommodate potential breaking changes.
There are a couple fields getting removed from Argo but not from KFP in this PR. Because it will need changes on production code, however, I am not sure whether changing it is desired.
https://github.com/zijianjoy/pipelines/blob/cf192ba4cf8c66a020ff64ee6cb6804238e0bd2d/frontend/third_party/argo-ui/argo_template.ts#L54
It is being referenced in https://github.com/zijianjoy/pipelines/blob/master/frontend/src/lib/StatusUtils.ts#L113-L115, but I am not sure if it is okay to remove.
https://github.com/zijianjoy/pipelines/blob/master/frontend/third_party/argo-ui/argo_template.ts#L557
It seems to be important field which is needed in Parser: https://github.com/zijianjoy/pipelines/blob/master/frontend/src/lib/StaticGraphParser.ts#L182
https://github.com/zijianjoy/pipelines/blob/master/frontend/src/lib/ParserUtils.ts#L39
Checklist:
Do you want this pull request (PR) cherry-picked into the current release branch?
Learn more about cherry-picking updates into the release branch.