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

Feature/text model metadata api #456

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ceruleandeep
Copy link

This is the second incremental PR for #452

I appreciate your review and am happy to revise according to your feedback.

The upshot of this PR would be that model reference data being served from the API is far more rigorously structured than the data being consumed from the json definition files, and somewhat more structured than the data being passed around internally. It would be possible later to extend typing and validation to the point of ingestion, or move the source of truth from JSON into the database. Accuracy and ease of maintenance is important for ongoing viability of #452.


Add metadata arg to /v2/status/models endpoint to request model metadata for available models

Add list and detail endpoints for known model metadata at /v2/knownmodels /v2/knownmodels/string:model_name

Refactor model_reference.py

@tazlin tazlin added the allow-ci A PR with this label will run through CI. label Sep 27, 2024
Add `metadata` arg to /v2/status/models endpoint to request model metadata for available models

Refactor model_reference.py
Copy link
Member

@db0 db0 left a comment

Choose a reason for hiding this comment

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

Some reorg changes, but I don't see anything seriously wrong. Can you also see if you can test this in python 3.9 to see if the added typing throws an exception?

@@ -1278,6 +1279,232 @@ def __init__(self, api):
),
},
)
self.response_model_known_model_md = api.model(
Copy link
Member

Choose a reason for hiding this comment

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

There's a separate file for image and text worker models. kobold_v2.py and stable_v2.py. You need to store the type-specific models in the right file to keep things somewhat organized.

Common data in both types should stay in v2.py and then use model inheritance to extend for each type.

help=(
"If 'known', only show stats for known models in the model reference. "
"If 'custom' only show stats for custom models. "
"If 'all' shows stats for all models."
),
location="args",
)
get_parser.add_argument(
Copy link
Member

@db0 db0 Sep 28, 2024

Choose a reason for hiding this comment

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

bool arguments in metadata are a bit buggy. A ?metadata=False query is treated as True, sadly, because it stupidly casts the string into a bool. To work around this, I suggest no default and then treat None as "False".

To properly work around this requires a bit of a hack by looking into the request query directly

Sadly the library we're using doesn't seem maintained well anymore :(

@@ -265,7 +265,7 @@ def worker_exists(worker_id):
return wc


def get_available_models(filter_model_name: str = None):
def get_available_models(filter_model_name: str = None) -> list[dict]:
Copy link
Member

Choose a reason for hiding this comment

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

Horde is still largely running in Python 3.9 so typing is not well supported there. I can't remember if this declaration works in 3.9

metadata: KnownModelRef


def known_models(model_type: str = None, filter_model_name: str = None, exact: bool = None) -> list[KnownModelsReturn]:
Copy link
Member

Choose a reason for hiding this comment

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

This should go inside the model_reference.py instead of at the root of the API file.

ref_json = DEFAULT_HORDE_IMAGE_COMPVIS_REFERENCE
if datetime.now(timezone.utc) <= datetime(2024, 9, 30, tzinfo=timezone.utc):
# Flux Beta
# I don't understand how this hack works, but perhaps HORDE_IMAGE_COMPVIS_REFERENCE is unset in prod
Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's unset in prod. This env var is to allow private hordes to use their own references. You can delete this comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
allow-ci A PR with this label will run through CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants