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

Slim kitfile spec #389

Merged
merged 4 commits into from
Jul 9, 2024
Merged

Slim kitfile spec #389

merged 4 commits into from
Jul 9, 2024

Conversation

amisevsk
Copy link
Contributor

@amisevsk amisevsk commented Jul 2, 2024

Description

Trim down the kitfile spec for simplicity (removed fields can be re-added later if we find a need for them):

  • Remove license field from model, dataset, and code, leaving only the top-level package license
  • Remove dataset references from model; since we only have one model now, datasets are assumed to apply to it
  • Remove processing field from datasets
  • Minor renames of internal fields to match their names in the Kitfile (e.g. TrainedModel -> Model)

This PR also changes how Kitfiles are parsed to print an error if an unknown field is present:

❯ kit pack -t test .
[ERROR] Failed to pack model kit: yaml: unmarshal errors:
  line 2: field manfiestVersion not found in type artifact.KitFile

(This could be cleaned up but the yaml package does not give us many easy options to do so right now)

Linked issues

Closes #239

@amisevsk amisevsk requested a review from gorkem July 2, 2024 17:22
Copy link
Contributor

@gorkem gorkem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need docs update to accompy this but otherwise looks good.

@amisevsk
Copy link
Contributor Author

amisevsk commented Jul 4, 2024

Turns out I had some copy-paste typos in the integration tests and this caught them, so we're already ahead :D

@gorkem
Copy link
Contributor

gorkem commented Jul 8, 2024

@amisevsk We have been discussing this with @bmicklea and he made the point that it is actually the top level license that is creating ambiguity and we should keep the licenses for code, model and datasets and deprecate the package level license.

@amisevsk
Copy link
Contributor Author

amisevsk commented Jul 8, 2024

I was kind of heading the other direction, I was considering turning the package-level license field into a list, so you could have

package:
  license: ["MIT", "CC BY-SA 4.0"]

or similar. At the end of the day, unpacking any of the layers should unpack a LICENSE file as well.

I'll update it to revert the license change (and then re-update #395 to match)

@amisevsk amisevsk force-pushed the slim-kitfile-spec branch 2 times, most recently from 78f7b73 to e642dec Compare July 9, 2024 17:58
@amisevsk
Copy link
Contributor Author

amisevsk commented Jul 9, 2024

Alright all license changes are reverted. We want licenses on models/datasets/code sections, and have to keep the package-level license field too since most existing modelkits use it.

@amisevsk
Copy link
Contributor Author

amisevsk commented Jul 9, 2024

Docs changes are in #395

amisevsk added 4 commits July 9, 2024 12:09
* Remove 'license' fields from each part, leaving only the top-level
  license
* Remove dataset references from model, since there's only one model now
* Remove preprocessing field from datasets
* Rename fields internally to match their name in the kitfile (package
  instead of kitfile, etc.)
Turns out restricting kitfiles to only known fields was useful for
showing some invalid Kitfiles.
@amisevsk amisevsk force-pushed the slim-kitfile-spec branch from e642dec to aba8411 Compare July 9, 2024 18:09
@amisevsk amisevsk merged commit c930e66 into jozu-ai:main Jul 9, 2024
3 checks passed
@amisevsk amisevsk deleted the slim-kitfile-spec branch July 9, 2024 18:14
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.

Generate JSON schema for Kitfile and use it to validate incoming Kitfiles
2 participants