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

Allow attaching tags to models; fix errors when pushing existing content to storage #8

Merged
merged 3 commits into from
Feb 12, 2024

Conversation

amisevsk
Copy link
Contributor

@amisevsk amisevsk commented Feb 9, 2024

Mainly, the goal of this PR is allow adding tags to builds:

jmm build -t <myrepo>:<mytag> .

This is required for jmm push to make sense

In order to make this work, however, it is necessary to make some changes to how artifacts are stored:

  • Folder used for storage is $JOZU_HOME/storage (default: .jozu/storage)
    • There are hooks for separating storage and configuration in the future, if e.g. we want to store artifacts in ~/.local/...
  • index.json now refers to the elements of one repository (c.f. image repositories; library/registry and library/registry2 are different repositories). This means the storage dir has multiple index.jsons. As far as I can tell, this is what is expected by the OCI image spec.
    • We'll probably want to improve this in the future to avoid duplication; e.g. via storing all blobs in one place and more intelligently wiring everything together. For now, this was the easiest way
    • Multiple tags for the same repo still live in the same index.json

If a tag is specified without a registry (e.g. my-repo instead of localhost:5000/my-repo), the default of localhost is used. For now, this will break if you use a tag like my/repo; my will be considered the registry.

I also fixed the bug where running build twice would fail becuase artifacts exist.

Add support for applying a tag when building via the --tag|-t flag. If
supplied, the build will be labelled with the tag.

As an early implementation, this will store the model's blobs _in a
different path_ than the default, matching the tag. For a tag like

  myregistry.com/my-organization/my-repo:my-tag

and a root config path of $HOME/.jozu/, blobs will be stored in

  $HOME/.jozu/storage/myregistry.com/my-organization/my-repo/<store>

This means that building the same model with different tags will
duplicate storage for now; this should be fixed in the future.

Summary of changes in this commit:

* Storage path is no longer .jozuStore appended to whatever config path
  is; instead Storage uses whatever path is provided and build command
  chooses 'storage' subdirecto of the config path
  * This is required to allow all tagged repos to be subdirectories of
    one base 'storage' directory
* If no tag is provided, blobs are stored in $JOZU_HOME/storage/ as
  before (change: storage instead of .jozuStore)
* If a tag is provided, it is applied as an annotation according to the
  OCI spec, using annotation 'org.opencontainers.image.ref.name'
* If a tag is provided but does not have a hostname, the default value
  of localhost is used.
Avoid error when pushing e.g. an already-existing blob into the local
storage.
@amisevsk amisevsk requested a review from gorkem February 9, 2024 22:16
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.

Just one comment to support multiple tags

return options, nil
}

func (flags *BuildFlags) AddFlags(cmd *cobra.Command) {
cmd.Flags().StringVarP(&flags.ModelFile, "file", "f", "", "Path to the model file")
cmd.Flags().StringVarP(&flags.FullTagRef, "tag", "t", "", "Tag for the model")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make this a StringArray which is the case for docker build -t command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done -- took a bit more work than expected, but also revealed that I was doing tagging wrong :)

Fix how tagging is done; instead of tagging manifests themselves, we
need to tag their entries in the index.json.

Additionally, add support for multiple tags specified at once, e.g.

jmm build -t myreg/myrepo:tag1,tag2,tag3
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.

LGTM

@amisevsk amisevsk merged commit 1f20bd0 into main Feb 12, 2024
1 check passed
@amisevsk amisevsk deleted the build-tag branch February 12, 2024 16:37
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