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

Lei/hack202205 #10

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Lei/hack202205 #10

wants to merge 19 commits into from

Conversation

eddyxu
Copy link
Contributor

@eddyxu eddyxu commented May 30, 2022

May hackathon for experimental embedding search

@eddyxu eddyxu force-pushed the lei/hack202205 branch 2 times, most recently from 7f017e0 to aaff37c Compare June 7, 2022 16:38
@eddyxu eddyxu requested a review from changhiskhan June 8, 2022 22:50
@eddyxu eddyxu marked this pull request as ready for review June 8, 2022 22:50
Copy link
Contributor

@changhiskhan changhiskhan left a comment

Choose a reason for hiding this comment

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

looks fine overall, a bunch of nits/questions

@@ -0,0 +1,18 @@
FROM postgres:14
Copy link
Contributor

Choose a reason for hiding this comment

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

do you want to copy this to Sophon as well so docker-compose spins up a rikai-pg enabled postgres?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, we will build a docker image for services.

@@ -1,9 +1,9 @@
FROM postgres:14
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 consolidate this vs the .github/Dockerfile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was a plan to have a playground docker released to docker registry. And another docker for CI.



class PgModel:
"""PostgreSQL Model"""
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think too many things are called "model" now. e.g.:

  1. we the actual pytorch/tf "model"
  2. we have the Rikai ModelType which gets assigned to self.model
  3. And this PgModel is also a "model"

This is probably worth a separate discussion tho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, thats true. This is more about a model adaptor

def schema(self) -> str:
return self.model.schema()

def args(self) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

don't quite understand what this does, but maybe it'll become clear later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was about building the arg list for the PG UDF.


def __repr__(self):
return f"PgModel({self.model})"
return f"PGModel(model={self.model})"
Copy link
Contributor

Choose a reason for hiding this comment

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

PgModel vs PGModel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

-- Test data
-- See images in rikai/tests/conftest.py
CREATE EXTENSION IF NOT EXISTS pgtap; -- For unit test
INSERT INTO ml.models (name, flavor, model_type, uri, options)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe add a description column?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point

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