feat: add arbitrary metadata GET/POST endpoints #9130

corban-beaird · 2024-04-09T04:31:07Z

Ticket

Description

Add support for storing arbitrary metadata associated with a given run. Additionally, this sets the groundwork for filtering runs based on flat_key-value pairs.

Test Plan

Automated integration testing for API
Automated unit testing

Checklist

Changes have been manually QA'd
User-facing API changes need the "User-facing API Change" label.
Release notes should be added as a separate file under docs/release-notes/.
See Release Note for details.
Licenses should be included for new code which was copied and/or modified from any external code.

netlify · 2024-04-09T04:31:30Z

✅ Deploy Preview for determined-ui canceled.

Name	Link
🔨 Latest commit	`2367ef8`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/666b1727e0c43a00087055c8

codecov · 2024-04-09T15:44:02Z

Codecov Report

Attention: Patch coverage is 54.29688% with 117 lines in your changes missing coverage. Please review.

Project coverage is 48.99%. Comparing base (e3d01c1) to head (2367ef8).
Report is 5 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #9130    +/-   ##
========================================
  Coverage   48.98%   48.99%            
========================================
  Files        1238     1240     +2     
  Lines      160518   160772   +254     
  Branches     2783     2784     +1     
========================================
+ Hits        78629    78763   +134     
- Misses      81714    81834   +120     
  Partials      175      175

Flag	Coverage Δ
backend	`43.91% <62.79%> (+0.05%)`	⬆️
harness	`63.80% <36.90%> (-0.06%)`	⬇️
web	`44.13% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
master/internal/api_experiment.go	`56.79% <100.00%> (+0.02%)`	⬆️
master/internal/db/postgres_trial.go	`62.61% <100.00%> (ø)`
master/pkg/model/experiment.go	`14.63% <100.00%> (+0.34%)`	⬆️
master/internal/api_runs.go	`63.19% <82.75%> (+0.89%)`	⬆️
harness/determined/core/_train.py	`39.83% <33.33%> (-1.08%)`	⬇️
master/internal/run/runs.go	`87.09% <87.09%> (ø)`
harness/determined/common/api/bindings.py	`40.21% <37.33%> (-0.02%)`	⬇️
master/internal/db/postgres_runs.go	`0.00% <0.00%> (ø)`

... and 3 files with indirect coverage changes

NicholasBlaskey

My main comment is about the product question

master/internal/api_runs.go

master/static/migrations/20240409101349_add-arbitrary-metadata-table.tx.up.sql

master/internal/api_runs.go

master/static/migrations/20240409101349_add-arbitrary-metadata-table.tx.up.sql

harness/determined/core/_train.py

azhou-determined · 2024-05-02T22:35:29Z

is there a way for someone to delete keys from the metadata or is that out of scope for this?

corban-beaird · 2024-05-02T22:42:33Z

is there a way for someone to delete keys from the metadata or is that out of scope for this?

@azhou-determined
Not currently, I do believe it's out of scope for this PR, but I don't see a reason why we shouldn't add it to the backlog of items to knock out with this project.

How granular are we wanting to allow a metadata to be deleted? I would be in favor of allowing requests to be made on at a key level + the ability to delete all arbitrary metadata on a given run at once.

azhou-determined · 2024-05-03T19:10:37Z

is there a way for someone to delete keys from the metadata or is that out of scope for this?

@azhou-determined Not currently, I do believe it's out of scope for this PR, but I don't see a reason why we shouldn't add it to the backlog of items to knock out with this project.

How granular are we wanting to allow a metadata to be deleted? I would be in favor of allowing requests to be made on at a key level + the ability to delete all arbitrary metadata on a given run at once.

i guess if we're going with the "only set/update, never add/merge" approach, we don't really need to surface a way to delete. as you mentioned, set_metadata(metadata=None) seems logical to me. i think we should have remove if we go with add though.

harness/determined/core/_train.py

azhou-determined

python side lgtm

NicholasBlaskey

looks good to me

I think my only concern is the code duplication and the future of this. I would talk with @AmanuelAaron and see if we can design something that works for both cases?

master/internal/api_runs.go

master/internal/db/postgres_runs.go

master/internal/api_runs.go

master/internal/db/postgres_runs.go

NicholasBlaskey · 2024-06-06T20:21:21Z

master/internal/run/runs.go

+)
+
+// FlattenMetadata flattens a nested map of run metadata into a list of RunMetadataIndex entries.
+func FlattenMetadata(


@AmanuelAaron just landed a PR that boils down to wanting to index hyperparameters

Is there a way we could only have one way of flattening / indexing JSONB in our code?

I would be down to make this more general & push it into a util file that can be shared!

NicholasBlaskey · 2024-06-06T20:27:57Z

master/internal/run/runs.go

+}
+
+// parseMetadataValueType converts a value to a string and returns the type of the value.
+func parseMetadataValueType(rawValue any) (value string, valueType string) {


summary metrics does this same kinda thing

determined/master/internal/db/postgres_trial.go

Line 722 in 84299a6

switch metricValue := metric.AsInterface().(type) {

would be cool if we only had one place where we did this, since it is easy to get wrong. I would check to see what happens if you report NaN in python code if it becomes a float

Sounds great! I'm down to improve the reuse!

master/static/migrations/20240520133440_add-arbitrary-metadata-table.tx.up.sql

…ferent table

…tory

…on trials view

…p if run has no metadata instead of error

…ata in Core API

…-table.tx.up.sql Co-authored-by: Nick Blaskey <[email protected]>

…ore API function to remove redundant "metadata" key

cla-bot bot added the cla-signed label Apr 9, 2024

corban-beaird requested a review from AmanuelAaron April 9, 2024 04:31

corban-beaird force-pushed the corban-arbitrary-metadata-get-post branch from 77eaf89 to a3d5165 Compare April 9, 2024 15:59

corban-beaird requested a review from NicholasBlaskey April 9, 2024 16:08

corban-beaird marked this pull request as ready for review April 9, 2024 16:16

corban-beaird requested a review from a team as a code owner April 9, 2024 16:16

NicholasBlaskey reviewed Apr 10, 2024

View reviewed changes

master/internal/api_runs.go Outdated Show resolved Hide resolved

master/static/migrations/20240409101349_add-arbitrary-metadata-table.tx.up.sql Outdated Show resolved Hide resolved

master/internal/api_runs.go Outdated Show resolved Hide resolved

corban-beaird requested a review from NicholasBlaskey April 11, 2024 15:27

corban-beaird requested a review from a team as a code owner April 11, 2024 16:02

corban-beaird requested a review from azhou-determined April 11, 2024 16:02

corban-beaird force-pushed the corban-arbitrary-metadata-get-post branch from 72c28d4 to a323e14 Compare April 17, 2024 22:10

AmanuelAaron reviewed Apr 17, 2024

View reviewed changes

master/static/migrations/20240409101349_add-arbitrary-metadata-table.tx.up.sql Outdated Show resolved Hide resolved

corban-beaird requested a review from AmanuelAaron April 19, 2024 14:43

stoksc reviewed Apr 26, 2024

View reviewed changes

master/static/migrations/20240409101349_add-arbitrary-metadata-table.tx.up.sql Outdated Show resolved Hide resolved

azhou-determined reviewed May 2, 2024

View reviewed changes

harness/determined/core/_train.py Outdated Show resolved Hide resolved

harness/determined/core/_train.py Show resolved Hide resolved

corban-beaird force-pushed the corban-arbitrary-metadata-get-post branch 4 times, most recently from 92f7cde to 05f3fbc Compare May 22, 2024 15:50

corban-beaird force-pushed the corban-arbitrary-metadata-get-post branch from 8e16475 to 5b2bad3 Compare May 30, 2024 18:06

corban-beaird requested a review from azhou-determined May 30, 2024 19:25

corban-beaird assigned AmanuelAaron and azhou-determined May 30, 2024

corban-beaird force-pushed the corban-arbitrary-metadata-get-post branch from 5b2bad3 to a447850 Compare June 3, 2024 15:38

azhou-determined reviewed Jun 3, 2024

View reviewed changes

harness/determined/core/_train.py Outdated Show resolved Hide resolved

azhou-determined reviewed Jun 3, 2024

View reviewed changes

harness/determined/core/_train.py Outdated Show resolved Hide resolved

corban-beaird requested a review from azhou-determined June 6, 2024 16:17

azhou-determined approved these changes Jun 6, 2024

View reviewed changes

corban-beaird force-pushed the corban-arbitrary-metadata-get-post branch from 2ad69b4 to 08a9ed5 Compare June 6, 2024 20:04

corban-beaird unassigned azhou-determined Jun 6, 2024

NicholasBlaskey approved these changes Jun 6, 2024

View reviewed changes

corban-beaird and others added 14 commits June 12, 2024 13:33

feat: add API & SDK support for arbitrary run metadata

a90a029

refactor: remove merge, add get logic, seperate run metadata into dif…

296f533

…ferent table

refactor: shift triggers & view work into new view_and_triggers direc…

7924b58

…tory

fix: adjust get_metadata sdk response

4a64fb8

fix: avoid summary_metric FOR UPDATE error due to nullable left join …

ad9d473

…on trials view

chore: ensure get_metadata return type matches

17ea05d

refactor: raise error instead of assert in Core API & return empty ma…

f9acafe

…p if run has no metadata instead of error

refactor: use bindings and return updated metadata when setting metad…

70dc902

…ata in Core API

refactor: correct binding usage & responses

33ab5ba

refactor: adjust return type to optional

112983a

chore: cleanup after rebase

6f3681e

Update master/static/migrations/20240520133440_add-arbitrary-metadata…

a326870

…-table.tx.up.sql Co-authored-by: Nick Blaskey <[email protected]>

chore: cleanup returns and remove dead code

4ecf30c

refactor: transition to explicit value columns for indexing, adjust C…

ca6c095

…ore API function to remove redundant "metadata" key

corban-beaird force-pushed the corban-arbitrary-metadata-get-post branch from f034c11 to ca6c095 Compare June 12, 2024 20:17

corban-beaird added 2 commits June 12, 2024 14:17

chore: clean up proto post rebase

1704c3d

chore: bump migration

2367ef8

corban-beaird merged commit d44013c into main Jun 13, 2024
84 of 98 checks passed

corban-beaird deleted the corban-arbitrary-metadata-get-post branch June 13, 2024 17:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add arbitrary metadata GET/POST endpoints #9130

feat: add arbitrary metadata GET/POST endpoints #9130

corban-beaird commented Apr 9, 2024 •

edited

Loading

netlify bot commented Apr 9, 2024 •

edited

Loading

codecov bot commented Apr 9, 2024 •

edited

Loading

NicholasBlaskey left a comment

azhou-determined commented May 2, 2024

corban-beaird commented May 2, 2024

azhou-determined commented May 3, 2024

azhou-determined left a comment

NicholasBlaskey left a comment

NicholasBlaskey Jun 6, 2024

corban-beaird Jun 6, 2024

NicholasBlaskey Jun 6, 2024

corban-beaird Jun 6, 2024

feat: add arbitrary metadata GET/POST endpoints #9130

feat: add arbitrary metadata GET/POST endpoints #9130

Conversation

corban-beaird commented Apr 9, 2024 • edited Loading

Ticket

Description

Test Plan

Checklist

netlify bot commented Apr 9, 2024 • edited Loading

✅ Deploy Preview for determined-ui canceled.

codecov bot commented Apr 9, 2024 • edited Loading

Codecov Report

NicholasBlaskey left a comment

Choose a reason for hiding this comment

azhou-determined commented May 2, 2024

corban-beaird commented May 2, 2024

azhou-determined commented May 3, 2024

azhou-determined left a comment

Choose a reason for hiding this comment

NicholasBlaskey left a comment

Choose a reason for hiding this comment

NicholasBlaskey Jun 6, 2024

Choose a reason for hiding this comment

corban-beaird Jun 6, 2024

Choose a reason for hiding this comment

NicholasBlaskey Jun 6, 2024

Choose a reason for hiding this comment

corban-beaird Jun 6, 2024

Choose a reason for hiding this comment

corban-beaird commented Apr 9, 2024 •

edited

Loading

netlify bot commented Apr 9, 2024 •

edited

Loading

codecov bot commented Apr 9, 2024 •

edited

Loading