fix(backend): Fix "Unknown execution spec" error. Fixes #8256 #8287
Conversation
| @@ -1124,7 +1124,7 @@ func (r *ResourceManager) ReadArtifact(runID string, nodeID string, artifactName | |||
| if run.WorkflowRuntimeManifest == "" { | |||
| return nil, util.NewInvalidInputError("read artifact from run with v2 IR spec is not supported") | |||
| } | |||
| execSpec, err := util.NewExecutionSpec([]byte(run.WorkflowRuntimeManifest)) | |||
There was a problem hiding this comment.
nice catch and this is the only place that calls NewExecutionSpec() which assumes the content is YAML. The content is in JSON format. sorry that I didn't catch the bug when doing the abstraction interface.
There was a problem hiding this comment.
there is a TestReadArtifact_Succeed test case and it doesn't fail. I guess the testing data can pass the yaml.Unmarshal magically. thinking if we can update the test case to catch up with this kind of error. I can work on that.
There was a problem hiding this comment.
which assumes the content is YAML. The content is in JSON format
I had the impression that because YAML is a superset of JSON, normally YAML serialization/deserialization libraries can handle both YAML and JSON (Didn't check if that's the case here, I probably got this impression from some Python library)
there is a TestReadArtifact_Succeed test case and it doesn't fail.
I noticed the test as well. I think in my debugging of KFP runtime, run.WorkflowRuntimeManifest at this point doesn't carry the TypeMeta part. It was unclear to me that if it should at this point, or maybe the type needs to be told (by passing util.ArgoWorkflow) like the rest NewExeuctionSpec* calls. The latter seems making sense to me.
There was a problem hiding this comment.
yea, I would say it should carry TypeMeta and also specify util.ArgoWorkflow. at least so far we haven't supported dynamically switching orchestration engine yet :-p
|
Merging this to unblock the release. /approve |
|
[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:
Approvers can indicate their approval by writing |
|
@chensun just want to update you on one doubt we had earlier:
It's from persistence agent. Somehow, the |
Description of your changes:
Fixes #8256
Manually verified the change resolves the aforementioned issue:
Checklist: