-
Notifications
You must be signed in to change notification settings - Fork 253
SUBMARINE-1002. Submarine model management database and database sdk #731
Conversation
@jeff-901 help me review this PR. Thanks. |
DROP TABLE IF EXISTS `metrics`; | ||
CREATE TABLE `metrics` ( | ||
`id` varchar(64) NOT NULL COMMENT 'Id of the Experiment', | ||
`key` varchar(190) NOT NULL COMMENT 'Metric key: `String` (limit 190 characters). Part of *Primary Key* for ``metrics`` table.', | ||
`value` float NOT NULL COMMENT 'Metric value: `Float`. Defined as *Non-null* in schema.', | ||
`worker_index` varchar(32) NOT NULL COMMENT 'Metric worker_index: `String` (limit 32 characters). Part of *Primary Key* for\r\n ``metrics`` table.', | ||
`timestamp` bigint(20) NOT NULL COMMENT 'Timestamp recorded for this metric entry: `BigInteger`. Part of *Primary Key* for ``metrics`` table.', | ||
`step` bigint(11) NOT NULL COMMENT 'Step recorded for this metric entry: `BigInteger`.', | ||
`is_nan` BOOLEAN NOT NULL COMMENT 'True if the value is in fact NaN.', | ||
PRIMARY KEY (`id`, `key`, `timestamp`, `worker_index`) | ||
) ENGINE=InnoDB DEFAULT CHARSET=utf8; | ||
|
||
DROP TABLE IF EXISTS `params`; | ||
CREATE TABLE `params` ( | ||
`id` varchar(64) NOT NULL COMMENT 'Id of the Experiment', | ||
`key` varchar(190) NOT NULL COMMENT '`String` (limit 190 characters). Part of *Primary Key* for ``params`` table.', | ||
`value` varchar(32) NOT NULL COMMENT '`String` (limit 190 characters). Defined as *Non-null* in schema.', | ||
`worker_index` varchar(32) NOT NULL COMMENT '`String` (limit 32 characters). Part of *Primary Key* for\r\n ``metrics`` table.', | ||
PRIMARY KEY (`id`, `key`, `worker_index`) | ||
) ENGINE=InnoDB DEFAULT CHARSET=utf8; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do metrics and params id need to be foreign key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds great to add the foreign key constraint. I will add it. Thanks
2490be6
to
2c1febe
Compare
@jeff-901 I have added the foreign key to both metrics and params. Help me review the following code, thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
4dae603
to
d84b7da
Compare
d84b7da
to
ec55144
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KUAN-HSUN-LI, Thanks.
LGTM. will merge it if more comments.
What is this PR for?
registered_models
,registered_model_tags
,model_versions
andmodel_version_tags
tables.params
andmetrics
tables fromsubmarine.sql
tosubmarine-model.sql
What type of PR is it?
[Feature]
Todos
What is the Jira issue?
https://issues.apache.org/jira/browse/SUBMARINE-1002
How should this be tested?
run the sdk unit tests
pytest --cov=submarine -vs -m "not e2e"
TestModelVersion and TestRegisteredModel are two relevant unit tests
Screenshots (if appropriate)
Questions: