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: model metadata query #1749

Merged
merged 32 commits into from
Dec 5, 2023
Merged

feature: model metadata query #1749

merged 32 commits into from
Dec 5, 2023

Conversation

fregataa
Copy link
Member

@fregataa fregataa commented Nov 29, 2023

related to #1066

Example model-definition.yaml file

models:
  - name: "simple-http-server"
    model_path: "/models"
    service:
      pre_start_actions:
        - action: run_command
          args:
            command: ["pip", "install", "-r", "/models/requirements.txt"]
      start_command:
        - python
        - /models/server.py
      port: 8000
      health_check:
        path: /health
        max_retries: 5
    metadata:
      author: "John Doe"
      version: 3
      created: "2019-01-01T00:00:00Z"
      last_modified: "2019-12-31T00:00:00Z"
      description: "This is a simple HTTP server"
      title: "Simple http server. Human readable name."
      task: "HTTP server"
      category: "langauge"
      label: 
        - "language"
      license: "Apache-2.0"
      min_resource:
        cpu: 1
        memory: 256m

Query Model Info connection(list)

query ModelInfoQuery {
    model_infos {
        edges {
            cursor
            node {
                name
                author
                title
                version
                created_at
                modified_at
                description
                task
                category
                label
                license
                min_resource
            }
        }
        count
        pageInfo {
            hasNextPage
            hasPreviousPage
        }
    }
}

Query single Model Info node

query ModelInfoQuery {
    model_info(id: "TW9kZWxJbmZvOjQ4M2RiY2Q5LTgxOGItNGJhYS1iMWI1LTFlNTY0NDI1NTczMw==") {
        name
        author
        title
        version
        created_at
        modified_at
        description
        task
        category
        label
        license
        min_resource
    }
}

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version
  • Mention to the original issue
  • Documentation
  • API server-client counterparts (e.g., manager API -> client SDK)
  • Test case(s) to demonstrate the difference of before/after

@fregataa fregataa added this to the 24.03 milestone Nov 29, 2023
@fregataa fregataa self-assigned this Nov 29, 2023
@github-actions github-actions bot added comp:manager Related to Manager component size:L 100~500 LoC labels Nov 29, 2023
@github-actions github-actions bot added the require:db-migration Automatically set when alembic migrations are added or updated label Nov 30, 2023
@github-actions github-actions bot added size:XL 500~ LoC and removed size:L 100~500 LoC labels Nov 30, 2023
Copy link
Contributor

@agatha197 agatha197 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@inureyes inureyes left a comment

Choose a reason for hiding this comment

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

LGTM

@inureyes inureyes added type:feature Add new features effort:hard Need to understand many components / a large extent of contextual or historical information. impact:visible This change is visible to users. platform:general General platform issues. Most issues are general. urgency:4 As soon as feasible, implementation is essential. labels Dec 5, 2023
Copy link
Member

@achimnol achimnol left a comment

Choose a reason for hiding this comment

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

Please fix some code-level, naming issues.

if group_type == ProjectType.MODEL_STORE:
if params["permission"] != VFolderPermission.READ_WRITE:
raise InvalidAPIParameters(
"Setting custom permission is not supported for model store VFolder"
Copy link
Member

Choose a reason for hiding this comment

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

When mentioned in the middle of sentences, use "vfolder", not "VFolder".

@@ -450,7 +454,7 @@ async def create(request: web.Request, params: Any) -> web.Response:
if group_uuid is not None:
ownership_type = "group"
quota_scope_id = QuotaScopeID(QuotaScopeType.PROJECT, group_uuid)
if not request["is_admin"]:
if not request["is_admin"] and group_type != ProjectType.MODEL_STORE:
Copy link
Member

Choose a reason for hiding this comment

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

Is this about forbidding model vfolder creation in the model store by a non-admin user?
Previously, project vfolder creation was already forbidden for non-admin users.
Is this additional condition strictly necessary? I think it may be better to just add comments.


def downgrade():
conn = op.get_bind()
conn.execute(text("DELETE FROM groups WHERE name = 'model-store'"))
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 refer the type column, not the name column.
(FYI: To use a keyword like type, user, etc. as a column name in SQL, surround it with double-quotes.)

@@ -111,6 +114,11 @@ class AssocGroupUserRow(Base):
group = relationship("GroupRow", back_populates="users")


class ProjectType(str, enum.Enum):
Copy link
Member

Choose a reason for hiding this comment

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

Use enum.StrEnum as of Python 3.11 and for the new codes that won't be backported to 22.09 or older.

@@ -75,6 +75,7 @@
t.Key("directory_based_usage", default=False): t.ToBool(),
t.Key("allow_custom_resource_allocation", default=True): t.ToBool(),
t.Key("edu_appname_prefix", default=""): t.String(allow_blank=True),
t.Key("support_model_store", default=True): t.ToBool(),
Copy link
Member

Choose a reason for hiding this comment

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

The name should be enable_model_store.
"Not support" means that we don't have the ability to serve the model store.
Here we should be clear that we DO support it but it's simply disabled when this field is set false.

@@ -30,6 +30,7 @@ connectionMode = "SESSION"
{% toml_field "allowCustomResourceAllocation" config["service"]["allow_custom_resource_allocation"] %}
{% toml_field "isDirectorySizeVisible" config["service"]["is_directory_size_visible"] %}
{% toml_field "eduAppNamePrefix" config["service"]["edu_appname_prefix"] %}
{% toml_field "supportModelStore" config["service"]["support_model_store"] %}
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

@@ -121,6 +121,29 @@
),
}
),
t.Key("info"): t.Null | t.Dict(
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest using a more concrete field name like "metadata" or "card".

@github-actions github-actions bot added the comp:storage-proxy Related to Storage proxy component label Dec 5, 2023
@fregataa fregataa requested a review from achimnol December 5, 2023 05:47
@fregataa fregataa added this pull request to the merge queue Dec 5, 2023
Merged via the queue into main with commit a4f62f9 Dec 5, 2023
25 checks passed
@fregataa fregataa deleted the feature/model-card-ui branch December 5, 2023 07:10
mirageoasis pushed a commit that referenced this pull request Dec 8, 2023
Co-authored-by: Jeongkyu Shin <[email protected]>
Co-authored-by: Kyujin Cho <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:manager Related to Manager component comp:storage-proxy Related to Storage proxy component comp:webserver Related to Web Server component effort:hard Need to understand many components / a large extent of contextual or historical information. impact:visible This change is visible to users. platform:general General platform issues. Most issues are general. require:db-migration Automatically set when alembic migrations are added or updated size:XL 500~ LoC type:feature Add new features urgency:4 As soon as feasible, implementation is essential.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants