Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to sigs.k8s.io/yaml instead of default; fixup jozufile struct #16

Merged
merged 2 commits into from
Feb 13, 2024

Conversation

amisevsk
Copy link
Contributor

Description

  • Switch to using sigs.k8s.io/yaml for YAML serialization/deserialization to avoid ambiguity between JSON and YAML representations of the Jozufile.
  • Add the omitempty tag to fields, to avoid serializing empty structs, and converts struct fields to pointers to ensure they can be nil/empty
  • Fixup struct fields; we were missing .package.license from the struct, even though it's present in the sample.

More info

gopkg.in/yaml.v3 uses a separate struct tag for YAML fields, leaving ambiguity between the JSON and YAML serialized format of a struct, unless struct tags are repeated:

type myStruct struct {
  myfield string `json:"myfield,omitempty",yaml:"myfield,omitempty"`
}

sigs.k8s.io/yaml instead reuses json struct tags, allowing for one specification to apply to both formats.

Prior to this PR, the jozufile.MarshalToJSON method would use capitalized fields instead of lowercase:

Before
{
  "ManifestVersion": "1.0",
  "Package": {
    "Name": "Densenet-ONNX",
    "Version": "",
    "Description": "Example using onnx.",
    "Authors": [
      "Gorkem"
    ]
  },
  "Code": null,
  "DataSets": null,
  "Models": [
    {
      "Name": "onnx Model",
      "Path": "./model",
      "Framework": "sklearn:1.3.0",
      "Version": "1.2",
      "Description": "Example model using onnx",
      "License": "Apache-2.0",
      "Training": {
        "DataSet": "",
        "Parameters": null
      },
      "Validation": {
        "DataSet": "",
        "Metrics": null
      }
    }
  ]
}
After
{
  "manifestVersion": "1",
  "package": {
    "name": "Densenet-ONNX",
    "description": "Example using onnx.",
    "license": "Apache-2.0",
    "authors": [
      "Gorkem"
    ]
  },
  "models": [
    {
      "name": "onnx Model",
      "path": "./model",
      "framework": "sklearn:1.3.0",
      "version": "1.2",
      "description": "Example model using onnx",
      "license": "Apache-2.0"
    }
  ]
}

Switch to using sigs.k8s.io/yaml for YAML serialization/deserialization
to avoid ambiguity between JSON and YAML representations of the
Jozufile.

gopkg.in/yaml.v3 uses a separate struct tag for YAML fields, leaving
ambiguity between the JSON and YAML serialized format of a struct,
unless struct tags are repeated:

type myStruct struct {
  myfield string `json:"myfield,omitempty",yaml:"myfield,omitempty"`
}

sigs.k8s.io/yaml instead reuses json struct tags, allowing for one
specification to apply to both formats.

In addition, this commit adds the `omitempty` tag to fields, to avoid
serializing empty structs, and converts struct fields to pointers to
ensure they can be nil/empty
@amisevsk amisevsk requested a review from gorkem February 13, 2024 19:15
@amisevsk
Copy link
Contributor Author

Note this change probably invalidate existing built artifacts; configs (and thus manifests) will change here. Old stores should be cleared after this PR is merged: rm -rf ~/.jozu/storage

The Yaml spec defines different scalar types, and unquoted strings are
parsed as numbers if the string can be parsed as a number. To ensure a
number is parsed as a string, it has to be enclosed in quotes or include
some character than cannot be in a number.
@amisevsk amisevsk merged commit ed9f0d0 into main Feb 13, 2024
1 check passed
@amisevsk amisevsk deleted the yaml-format branch February 13, 2024 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants