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

abstracting model downloads and file-normalization to models dir #190

Conversation

Gregory-Pereira
Copy link
Collaborator

@Gregory-Pereira Gregory-Pereira commented Apr 7, 2024

Changes:

  • /models
    • models should be downloaded from one place, the Makefile in the /models directory
    • models directory should house all models instead of the various model_server directories
  • /model_servers
    • downloads will be now be called from the a local ms make target which will call download-model with some prepathed information for tests, users responsible for their own models.
    • you should be able to test with all env variables, REGISTRY, IMAGE_NAME, MODEL_NAME, MODEL_PATH and PORT
    • instituted common directory for all model_servers for building, running, running tests, etc.
  • /.github/workflows
    • github workflows should call Makefiles for 2 reasons:
      • --> increase testing coverage
      • --> test how we expect users to interact with the repo

@Gregory-Pereira Gregory-Pereira marked this pull request as draft April 7, 2024 20:12
@Gregory-Pereira Gregory-Pereira force-pushed the abstract-model-downloads-to-model-dir branch 3 times, most recently from baa0b3b to 4b754a0 Compare April 8, 2024 01:52
@Gregory-Pereira Gregory-Pereira marked this pull request as ready for review April 8, 2024 01:55
@Gregory-Pereira Gregory-Pereira force-pushed the abstract-model-downloads-to-model-dir branch 6 times, most recently from 6ffe7e0 to 10862f1 Compare April 8, 2024 03:46
@Gregory-Pereira
Copy link
Collaborator Author

Its RFR @rhatdan @sallyom @lmilbaum

@lmilbaum
Copy link
Collaborator

lmilbaum commented Apr 8, 2024

One comment caught my eye. ghcr.io is used for testing the artifacts before they are promoted to quay.io. Hence, would you mind checking that the push pipelines are pushing the model servers container images to ghcr.io. I think that one is broken. Probably caused by the changes I merged yesterday.

models/Makefile Show resolved Hide resolved
models/Makefile Outdated Show resolved Hide resolved
@Gregory-Pereira Gregory-Pereira force-pushed the abstract-model-downloads-to-model-dir branch from 10862f1 to 227eed4 Compare April 9, 2024 01:39
@Gregory-Pereira
Copy link
Collaborator Author

Changes since last review:

  • reworked model_servers to use a common Makefile
  • remove mode.file normalization / obfuscation in favor of actual model name
  • add logging

Just going to make sure the readmes line up with these changes, conflicts are resolved and tests passes and then I will tag people for re-review

@Gregory-Pereira Gregory-Pereira force-pushed the abstract-model-downloads-to-model-dir branch 2 times, most recently from 3a4d012 to 1c0bc90 Compare April 9, 2024 21:02
@sallyom
Copy link
Collaborator

sallyom commented Apr 9, 2024

We want to keep the model-servers separate from models - we'll either volume mount a model as an init container from a containerized model, as the quadlet pod yamls are doing now, or from a host-filesystem volume mount as podman-desktop AI Lab does with the sample applications - this is to keep everything as pluggable as possible at both build time and runtime.
We made the change to use the generic model.file instead of the actual model name for the same reason, to be more flexible & pluggable - with the host filesystem mount, this makes a bit less sense but the source of truth is the Containerfile in this case, where you clearly document where & which model is being downloaded. (With containerized models, you can also show this with image metadata). These are also sample recipes and it's assumed that they won't be lifted exactly as/is into production, but the purpose is to provide a clear example of how to set up different AI applications to build on.

@Gregory-Pereira
Copy link
Collaborator Author

Gregory-Pereira commented Apr 9, 2024

We want to keep the model-servers separate from models - we'll either volume mount a model as an init container from a containerized model, as the quadlet pod yamls are doing now, or from a host-filesystem volume mount as podman-desktop AI Lab does with the sample applications - this is to keep everything as pluggable as possible at both build time and runtime.

For the most part this PR keeps with this isolation, that being said I will need to adapt it to the new "model-as-a-container" paradigm, but ill try to rework it to support both.

We made the change to use the generic model.file instead of the actual model name for the same reason, to be more flexible & pluggable - with the host filesystem mount, this makes a bit less sense but the source of truth is the Containerfile in this case, where you clearly document where & which model is being downloaded. (With containerized models, you can also show this with image metadata).

I am all for this concept of normalizing as much as we can to make models easily swappable, but I also feel that @MichaelClifford did have some good points about not calling it model.file. Personally, since we already have to specifiy the name of the model I dont think we lose anything by having the model mounted at /standard/mount/path/<model-name> but I would like to hear more takes on this.

@Gregory-Pereira
Copy link
Collaborator Author

holding this to focus on the funcitonal tests

@Gregory-Pereira Gregory-Pereira marked this pull request as draft April 11, 2024 04:59
@Gregory-Pereira Gregory-Pereira force-pushed the abstract-model-downloads-to-model-dir branch 2 times, most recently from 08355e2 to 1bc051c Compare April 12, 2024 03:17
@Gregory-Pereira Gregory-Pereira force-pushed the abstract-model-downloads-to-model-dir branch 5 times, most recently from 9d2074e to f6fd467 Compare April 12, 2024 04:42
@Gregory-Pereira Gregory-Pereira marked this pull request as ready for review April 12, 2024 04:54
@Gregory-Pereira Gregory-Pereira force-pushed the abstract-model-downloads-to-model-dir branch 4 times, most recently from b45f117 to fc57821 Compare April 12, 2024 16:00
@Gregory-Pereira Gregory-Pereira force-pushed the abstract-model-downloads-to-model-dir branch from fc57821 to d8fe927 Compare April 12, 2024 17:47
@Gregory-Pereira Gregory-Pereira merged commit 999093d into containers:main Apr 12, 2024
5 checks passed
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.

5 participants