opendatahub-io / ai-edge

ODH integration with AI at the Edge usecases
Apache License 2.0
9 stars 18 forks source link

Add list of servers for supported model formats #194

Closed grdryn closed 7 months ago

grdryn commented 8 months ago

JIRA: https://issues.redhat.com/browse/RHOAIENG-290

Description

The first commit here just gets yq to reformat the YAML files to its preferred style, so that the actual changes are clearer in the second commit. A YAML diff tool, such as https://yamldiff.com/, can be used to verify that the before and after of this first commit have the same YAML meaning. Adding the w=1 URL parameter to the files view here on GitHub will also minimize most of the formatting noise).

The second commit introduces a new file that has a list of mappings of model file formats to model servers. This file is just the source for the annotation value in the Pipeline and PipelineRun resources in the YAML files. When it's changed, the annotations can be updated using the following commands:

for i in aiedge-e2e.pipeline.yaml aiedge-e2e.pipelinerun.yaml ; do
    yq -i e ".metadata.annotations[\"opendatahub.io/supported-model-formats\"] = $(cat supported-model-formats.json | jq '@json')" $i
done

The list can be queried in a manner similar to the following:

oc get pipeline aiedge-e2e -o json \
   | jq '.metadata.annotations["opendatahub.io/supported-model-formats"] | fromjson | .[] | select(."model-format" == "onnx")'

How Has This Been Tested?

I successfully deployed this to a cluster, and successfully queried the list from the annotation using the command listed in the description above.

Merge criteria:

openshift-ci[bot] commented 8 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grdryn

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: - ~~[OWNERS](https://github.com/opendatahub-io/ai-edge/blob/main/OWNERS)~~ [grdryn] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
LaVLaS commented 8 months ago

The second commit introduces a new file that has a list of mappings of model file formats to model servers.

IMO, we don't need to maintain a separate file to host the mappings. In another comment I pointed out that explicit PipelineRun annotations are not needed if they are duplicates from the referenced Pipeline object.

See Tekton Propagation of Labels and Annotations

grdryn commented 8 months ago

IMO, we don't need to maintain a separate file to host the mappings.

@LaVLaS While we could just manually modify an inline JSON string in the YAML, I think there's benefit to maintaining it as a separate file, and having yq & jq manage it in the YAML: jq will make sure that it's valid JSON before serializing it into the string, and yq will help make sure that we don't accidentally break the YAML somehow when inserting it.

I did spend a little bit of time trying to just use yq, since it can work with JSON also now (we could maintain the separate file as YAML and then get yq to translate that to json for the annotation value), but I couldn't get it to work easily and didn't want to spend too much time on it.

LaVLaS commented 8 months ago

I think there's benefit to maintaining it as a separate file

Understandable. I wonder if that would be better suited to a Readme with just the links to you provided about model formats supported by Seldon and OpenVino.

Since we are only tracking the model servers : [model formats] that each Pipeline supports, do we need the external file since the info should be updated within the Pipeline and the respective PipelineRun will inherit it automatically? Also, if we do decide to maintain the external json file that can be automated to update the Pipeline and PipelineRun then we should add those to a script in the repo or Makefile for reproducibility

MarianMacik commented 8 months ago

/retest

MarianMacik commented 8 months ago

@grdryn thanks, the job's green now.

grdryn commented 7 months ago

We're going to defer this for now.