feat: summary metrics #6477
Merged
feat: summary metrics #6477
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
EXPLAIN ANALYZE output of 1000 trials |
NicholasBlaskey
force-pushed
the
unified_metrics_migration
branch
2 times, most recently
from
e228d37
to
6c18e7f
Compare
nrajanee
reviewed
Apr 6, 2023
| }, | ||
| }, | ||
| TotalBatches: 1, | ||
| EndTime: time.Now().AddDate(0, 0, -1), |
There was a problem hiding this comment.
question: is it past because of the end time here?
There was a problem hiding this comment.
Yeah the query only looks at end time to know if we need to recalculate a trial's summary metrics.
|
|
||
| ALTER TABLE trials | ||
| ADD COLUMN IF NOT EXISTS summary_metrics jsonb NOT NULL DEFAULT '{}', | ||
| ADD COLUMN IF NOT EXISTS summary_metrics_timestamp timestamptz DEFAULT NULL; |
There was a problem hiding this comment.
question: do we need DEFAULT NULL here? I think the default will be null anyway?
There was a problem hiding this comment.
Yeah we don't need it, removed it.
| ADD COLUMN IF NOT EXISTS summary_metrics_timestamp timestamptz DEFAULT NULL; | ||
|
|
||
| -- Invalidate summary_metrics_timestamp for trials that have a metric added since. | ||
| WITH max_training as ( |
There was a problem hiding this comment.
suggestion: this sql file is a little difficult to read because it's so long. it might be good to have a couple of comments in the PR or in the file about what's going on.
There was a problem hiding this comment.
Added some high level comments of what each CTE does.
| m[strconv.Itoa(j)] = rand.Float64() //nolint: gosec | ||
| } | ||
|
|
||
| metrics = append(metrics, step{ |
There was a problem hiding this comment.
question: why aren't you using AddTrainingMetrics and AddValidationMetrics here? if the goal here is to generate metrics for a trial can we make use of the functions from here: #6222?
There was a problem hiding this comment.
It is a lot quicker to generate with data for the test with this. I was considering deleting this just needed it for a test and wanted to make sure the test database generation was reasonable.
Its quicker since it uses batch inserts, skips batch metrics, skips rbac checks related queries, and also skips archiving checks.
NicholasBlaskey
commented
Apr 7, 2023
| WHEN (metrics->'avg_metrics'->name)::text = '"Infinity"'::text THEN 'number' | ||
| WHEN (metrics->'avg_metrics'->name)::text = '"-Infinity"'::text THEN 'number' | ||
| WHEN (metrics->'avg_metrics'->name)::text = '"NaN"'::text THEN 'number' | ||
| ELSE jsonb_typeof(metrics->'avg_metrics'->name) |
There was a problem hiding this comment.
came up in testing top trials by metric
old code would sometimes let people get away with report the string "1.0" and we would cast it to the float 1.0 in some queries.
should this migration and ingestion code treat "1.0" as 1.0?
There was a problem hiding this comment.
no, I don't think this is a kind of behavior we should support. that doesn't seem to me like something a user would rely upon.
NicholasBlaskey
force-pushed
the
unified_metrics_migration
branch
from
f5c323e
to
e23f9c4
Compare
|
@ioga should be ready for review now |
ioga
approved these changes
Apr 14, 2023
master/internal/db/postgres_trial.go
Outdated
| "count": 1, | ||
| } | ||
| } else { | ||
| // Check if the metric had a non numeric value in the past. |
| math.Max(summaryMetric["max"].(float64), metricValue)) | ||
| summaryMetric["sum"] = replaceSpecialFloatsWithString( | ||
| summaryMetric["sum"].(float64) + metricValue) | ||
| // Go parsing odditity treats JSON whole numbers as floats. |
There was a problem hiding this comment.
it's javascript fault: there're no integer values, only floating point numbers
4 tasks
|
@ioga added types |
NicholasBlaskey
force-pushed
the
unified_metrics_migration
branch
from
084de9d
to
7829e2c
Compare
tayritenour
pushed a commit
to tayritenour/determined
that referenced
this pull request
Apr 25, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Adds summary metrics migration and ingestion code.
Test Plan
Integration test plus test database generated for benchmarking
Spot test run an experiment and look in the database for
summary_metricsCommentary (optional)
Test cluster generated with 10k trials each with 25k steps and 2.5k validations reporting 5 different metrics.
This should be a decent overestimate on what we expect.
156 secondsto compute which metrics will need to be incrementally updated (will run every migration)1.4 secondsto update 1 single trial13.8 secondsto update 10 trials3.0 minutesto update 100 trials39.4 minutesto update 1000 trials4.2 hourswhen the full database updatedQuery plan shown below
Checklist
docs/release-notes/.See Release Note for details.
Ticket