feat: Log Signal Exp Config and Monitoring #9947

jgongd · 2024-09-17T19:34:34Z

Ticket

MD-493, MD-494

Description

Update log_policies json schema. To avoid any disruption, both legacy log policy and modern log policy are supported.
Store the name of the policy to the DB if a matching pattern is found in the log.
Rename logSignal to logPolicyMatched in the webUI.

Test Plan

At the release party, testing this PR together with #9959 could make things easier.

Include below in the experiment config and verify if Test Signal are in run details page and run table.

log_policies:
 - name: "test"
   pattern: .*complete.*

Legacy log policy should still be accepted.

log_policies:
 - pattern: .*complete.*
   action:  
     type: cancel_retries

If the experiment config doesn't specify any log policies, it will use the default log policies.

Default log policies can be unset.

log_policies:
  - name: ECC Error
  - name: CUDA OOM

Checklist

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

netlify · 2024-09-17T19:34:57Z

✅ Deploy Preview for determined-ui canceled.

Name	Link
🔨 Latest commit	`57031d0`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/671ab73c167fbc000896541e

codecov · 2024-09-17T19:36:11Z

Codecov Report

Attention: Patch coverage is 41.61491% with 94 lines in your changes missing coverage. Please review.

Project coverage is 54.56%. Comparing base (deb3772) to head (57031d0).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
master/pkg/schemas/zgen_schemas.go	0.00%	40 Missing ⚠️
master/internal/logpattern/logpattern.go	5.40%	35 Missing ⚠️
master/pkg/schemas/expconf/log_pattern_config.go	88.05%	8 Missing ⚠️
.../src/pages/TrialDetails/Header/TrialHeaderLeft.tsx	20.00%	4 Missing ⚠️
...pkg/schemas/expconf/zgen_log_policies_config_v0.go	33.33%	2 Missing ⚠️
master/pkg/schemas/expconf/zgen_log_policy_v0.go	0.00%	2 Missing ⚠️
master/pkg/model/task_container_defaults.go	0.00%	1 Missing ⚠️
webui/react/src/pages/FlatRuns/columns.ts	0.00%	1 Missing ⚠️
webui/react/src/services/decoder.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9947   +/-   ##
=======================================
  Coverage   54.55%   54.56%           
=======================================
  Files        1267     1265    -2     
  Lines      159525   159601   +76     
  Branches     3637     3637           
=======================================
+ Hits        87035    87089   +54     
- Misses      72357    72379   +22     
  Partials      133      133

Flag	Coverage Δ
backend	`45.95% <42.10%> (+0.03%)`	⬆️
harness	`72.56% <ø> (ø)`
web	`54.02% <33.33%> (ø)`

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

Files with missing lines	Coverage Δ
master/internal/api_experiment.go	`57.18% <100.00%> (ø)`
master/internal/api_runs.go	`66.27% <100.00%> (ø)`
...ternal/configpolicy/postgres_task_config_policy.go	`91.66% <ø> (ø)`
master/pkg/model/task.go	`15.26% <ø> (ø)`
...ster/pkg/schemas/extensions/disallow_properties.go	`96.77% <ø> (ø)`
webui/react/src/types.ts	`99.70% <100.00%> (ø)`
master/pkg/model/task_container_defaults.go	`85.00% <0.00%> (+0.62%)`	⬆️
webui/react/src/pages/FlatRuns/columns.ts	`18.49% <0.00%> (ø)`
webui/react/src/services/decoder.ts	`21.32% <0.00%> (ø)`
...pkg/schemas/expconf/zgen_log_policies_config_v0.go	`33.33% <33.33%> (ø)`
... and 5 more

... and 4 files with indirect coverage changes

hamidzr · 2024-09-30T16:07:02Z

cc @determined-ai/backend for backend review. thank you

gt2345 · 2024-09-30T23:05:03Z

master/internal/trial.go

+				InformationalReason: err.Error(),
+			})
+		}
+


Can we please add some logs explaining why we want to clear signal when allocation exit fails

I don't follow why we would want to clear it. It seems the point of this change is to track when some exceptional event happen and present them in a more user friendly manner. Losing that we had an ECC error on node xyz002 seems important to surface still.

If a trial runs into an OOM error, it's caught in log and a signal will be shown in the UI. I think the signal only appears for a brief moment since the trial will restart quickly. This means user might not see there was an OOM error. You are right. I feel I shouldn't clear it at restart.

gt2345

Great work on extensive tests!

stoksc

i know i'm a little late to the party here but log_signal / signal is a really confusing word to me for what this is. is it too late to just call it "log_policy_name"? it makes sense to me that way, that users would name their log policies and look, by name, at which one fired on a given run.

other than that, i've left some general go-related comments and i'm not sure we should be breaking the experiment config but mostly this looks fine.

stoksc · 2024-10-17T00:28:17Z

master/pkg/schemas/expconf/log_pattern_config.go

 type LogPoliciesConfigV0 []LogPolicyV0

+// WithDefaults implements the Defaultable psuedointerface.
+func (b *LogPoliciesConfigV0) WithDefaults() *LogPoliciesConfigV0 {


Suggested change

func (b *LogPoliciesConfigV0) WithDefaults() *LogPoliciesConfigV0 {

func (b LogPoliciesConfigV0) WithDefaults() *LogPoliciesConfigV0 {

Don't use a pointer receiver here.

For WithDefaults to work, I think you must match the pointerness of the receiver and return value.

So probably want to return a non-pointer as well, I think.

stoksc · 2024-10-17T00:30:11Z

master/pkg/schemas/expconf/log_pattern_config.go

+	for _, p := range b {
+		if v, ok := patternToLp[p.RawPattern]; ok {
+			// Union merge actions
+			actions := make(set.Set[LogActionV0])


use set.New

stoksc · 2024-10-17T00:31:52Z

master/pkg/schemas/expconf/log_pattern_config.go

+	}
+	return b
+}
+
 // Merge implemenets the mergable interface.
 func (b LogPoliciesConfigV0) Merge(


can you add unit test this merge?

Added unit tests of this merge: https://github.com/determined-ai/determined/pull/9947/files#diff-e1c26ed8b923497a36fcb692a5cea7b555b2a34159efc0e49bbf9dae4cb0b11aR97-R212

stoksc · 2024-10-17T00:33:16Z

master/pkg/schemas/expconf/log_pattern_config.go

@@ -40,7 +75,8 @@ func (b LogPoliciesConfigV0) Merge(
 type LogPolicyV0 struct {
 	RawPattern string `json:"pattern"`

-	RawAction LogActionV0 `json:"action"`
+	RawActions []LogActionV0 `json:"actions,omitempty"`


did anyone explicitly ask for multiple actions? today the available actions seem mutually exclusive in terms of when you would want them to me, and this is a breaking config change for anyone using this featuer.

did anyone explicitly ask for multiple actions

The "signal" action shows up in the UI, but you might also want an action with a side-effect.

this is a breaking config change

We should be implementing shims to keep from breaking old configs.

@jgongd Do you have the examples of the desired config examples that we worked out together? I feel like @stoksc would benefit from seeing the overall goal here.

I was concerned that it might be a breaking change as well!

Desired config example:

log_policies: - name: ECC Error pattern: .*uncorrectable ECC error encountered.* action: exclude_node

Shim is implemented here.

Both legacy log policy and modern log policy are accepted now: https://github.com/determined-ai/determined/pull/9947/files#diff-ced209455ef7f0c29127fe9cc7a734f53cf0012d1268916b89afe0ed4f98ad17

Tested here.

stoksc · 2024-10-17T00:35:07Z

master/pkg/schemas/expconf/experiment_config.go

@@ -29,7 +29,7 @@ type ExperimentConfigV0 struct {
 	RawEnvironment              *EnvironmentConfigV0        `json:"environment"`
 	RawHyperparameters          HyperparametersV0           `json:"hyperparameters"`
 	RawLabels                   LabelsV0                    `json:"labels"`
-	RawLogPolicies              LogPoliciesConfigV0         `json:"log_policies"`
+	RawLogPolicies              *LogPoliciesConfigV0        `json:"log_policies"`


why change this to pointers to a slice throughout the PR?

Good point! A slice doesn't need a pointer.

stoksc · 2024-10-17T00:41:24Z

master/internal/trial.go

+				InformationalReason: err.Error(),
+			})
+		}
+


I don't follow why we would want to clear it. It seems the point of this change is to track when some exceptional event happen and present them in a more user friendly manner. Losing that we had an ECC error on node xyz002 seems important to surface still.

master/pkg/schemas/expconf/log_pattern_config.go

jgongd · 2024-10-18T23:26:14Z

master/pkg/model/task_container_defaults_test.go

 		},
 	}
 	require.Equal(t, expected, tcd)
 }

-func TestLogPatternPoliciesMerging(t *testing.T) {


Converted this test to a unit test log_policy.yaml::legacy_log_policies_merging

jgongd · 2024-10-18T23:38:53Z

schemas/test_cases/v0/unions.yaml

@@ -61,20 +61,6 @@
    type: directory
    container_path: /path/on/disk

- name: log action cancel_retries


Moved to log_policies.yaml

action is a legacy field now

gt2345 · 2024-10-24T16:15:40Z

master/pkg/model/experiment.go

@@ -522,6 +522,7 @@ type Run struct {
 	LogRetentionDays      *int16         `db:"log_retention_days"`
 	Metadata              map[string]any `db:"metadata" bun:"metadata,scanonly"`
 	LocalID               int            `db:"local_id"`
+	LogPolicyMatched      *string        `db:"log_policy_matched"`


Why do we want to add LogPolicyMatched to experiment model?

I thought /api/v1/runs query uses this model. But checking the code again, I see that it actually uses the go struct generated from message Flatrun in the proto file. Thank you, great catch!

stoksc · 2024-10-24T17:00:23Z

master/internal/logpattern/logpattern.go

-					); err != nil {
-						return fmt.Errorf("adding retry on different node: %w", err)
+				if policy.Name() != nil {
+					err = db.Bun().RunInTx(ctx, nil, func(ctx context.Context, tx bun.Tx) error {


ideally this wouldn't be in a separate transaction

Maybe i don’t understand the ideal approach you mentioned yet. My intention is to put two update queries in the same transaction, so they either both succeed or failed

sorry, i was totally unclear. i agree these two query should be in a transaction, but i think there are also writes in addRetryOnDifferentNode and addDontRetry that are in a separate transaction.

I see. Queries of one policy are either all succeed or all fail.

stoksc · 2024-10-24T18:37:38Z

master/pkg/schemas/expconf/log_pattern_config.go

+	var lat LogActionType
+	if err := json.Unmarshal(data, &lat); err == nil {
+		switch lat {
+		case LogActionTypeCancelRetries, LogActionTypeExcludeNode:


if the type isn't an expected one we end up fmt.Errorf'ing a nil err.

Add default to switch.

stoksc · 2024-10-24T18:39:47Z

lgtm, only had some non-blocking (i.e., please address but i dont need to review.. i need a better word for this), minor feedback

Co-authored-by: Ryan <[email protected]>

cla-bot bot added the cla-signed label Sep 17, 2024

jgongd force-pushed the jgong/feat-log-signal-exp-config branch from 8679a19 to 176454d Compare September 29, 2024 23:42

jgongd changed the title ~~Log Signal Exp Config and Data Storing~~ feat: Log Signal Exp Config and Data Storing Sep 29, 2024

jgongd requested a review from gt2345 September 30, 2024 01:20

jgongd marked this pull request as ready for review September 30, 2024 02:06

jgongd requested review from a team as code owners September 30, 2024 02:06

jgongd requested a review from hamidzr September 30, 2024 02:06

hamidzr removed their request for review September 30, 2024 16:05

gt2345 reviewed Sep 30, 2024

View reviewed changes

gt2345 approved these changes Sep 30, 2024

View reviewed changes

jgongd force-pushed the jgong/feat-log-signal-exp-config branch from 994f73f to 761f8f8 Compare October 10, 2024 04:17

jgongd requested a review from salonig23 October 10, 2024 04:20

jgongd changed the title ~~feat: Log Signal Exp Config and Data Storing~~ feat: Log Signal Exp Config and Monitoring Oct 10, 2024

stoksc reviewed Oct 17, 2024

View reviewed changes

jgongd force-pushed the jgong/feat-log-signal-exp-config branch 4 times, most recently from e4e6acd to eea42c6 Compare October 17, 2024 06:37

rb-determined-ai reviewed Oct 17, 2024

View reviewed changes

master/pkg/schemas/expconf/log_pattern_config.go Outdated Show resolved Hide resolved

jgongd force-pushed the jgong/feat-log-signal-exp-config branch from 8816f3b to 50b0ebe Compare October 18, 2024 22:11

jgongd commented Oct 18, 2024

View reviewed changes

jgongd force-pushed the jgong/feat-log-signal-exp-config branch 2 times, most recently from 1d9d9df to 8095267 Compare October 19, 2024 04:34

jgongd requested review from rb-determined-ai and stoksc October 19, 2024 04:37

jgongd and others added 17 commits October 23, 2024 21:27

lint

eae8064

Update default test

04bc0ee

Add Backward Compatibility

323d58f

action is a legacy field now

Address comment

2c69c90

for jerry

e984fc2

Address comments

e695022

fix tests

d66a9a2

lint

1781b38

fix bugs detected by log policies tests

feb2e64

clean up

efa27ec

update tests after rebase

531b78e

bug fix

a5285b7

comments

0075d86

Change Log Signal feature behaviors

ef6d5f3

Address comments

2ac7fdc

Address more comments

65fc50f

fix failed tests

f08317d

jgongd force-pushed the jgong/feat-log-signal-exp-config branch from 3110d2d to f08317d Compare October 24, 2024 01:35

gt2345 reviewed Oct 24, 2024

View reviewed changes

stoksc reviewed Oct 24, 2024

View reviewed changes

stoksc approved these changes Oct 24, 2024

View reviewed changes

jgongd added 3 commits October 24, 2024 16:47

Address more comments

77de8c9

move db migration file to top

193faa0

fix failed CI tests

57031d0

jgongd merged commit c71617c into main Oct 24, 2024
81 of 95 checks passed

jgongd deleted the jgong/feat-log-signal-exp-config branch October 24, 2024 21:43

thiagodallacqua-hpe pushed a commit that referenced this pull request Oct 28, 2024

feat: Log Signal Exp Config and Monitoring (#9947)

76debe5

Co-authored-by: Ryan <[email protected]>

thiagodallacqua-hpe pushed a commit that referenced this pull request Oct 28, 2024

feat: Log Signal Exp Config and Monitoring (#9947)

0ca179b

Co-authored-by: Ryan <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Log Signal Exp Config and Monitoring #9947

feat: Log Signal Exp Config and Monitoring #9947

jgongd commented Sep 17, 2024 •

edited

Loading

netlify bot commented Sep 17, 2024 •

edited

Loading

codecov bot commented Sep 17, 2024 •

edited

Loading

hamidzr commented Sep 30, 2024 •

edited

Loading

gt2345 Sep 30, 2024

stoksc Oct 17, 2024

jgongd Oct 18, 2024

gt2345 left a comment

stoksc left a comment

stoksc Oct 17, 2024

rb-determined-ai Oct 17, 2024

jgongd Oct 18, 2024

stoksc Oct 17, 2024

stoksc Oct 17, 2024

jgongd Oct 24, 2024

stoksc Oct 17, 2024

rb-determined-ai Oct 17, 2024

jgongd Oct 19, 2024 •

edited

Loading

stoksc Oct 17, 2024

jgongd Oct 17, 2024

stoksc Oct 17, 2024

jgongd Oct 18, 2024

jgongd Oct 18, 2024

gt2345 Oct 24, 2024

jgongd Oct 24, 2024

stoksc Oct 24, 2024

jgongd Oct 24, 2024

stoksc Oct 24, 2024

jgongd Oct 24, 2024

stoksc Oct 24, 2024

jgongd Oct 24, 2024

stoksc commented Oct 24, 2024 •

edited

Loading

	func (b LogPoliciesConfigV0) WithDefaults() LogPoliciesConfigV0 {
	func (b LogPoliciesConfigV0) WithDefaults() *LogPoliciesConfigV0 {

feat: Log Signal Exp Config and Monitoring #9947

feat: Log Signal Exp Config and Monitoring #9947

Conversation

jgongd commented Sep 17, 2024 • edited Loading

Ticket

Description

Test Plan

Checklist

netlify bot commented Sep 17, 2024 • edited Loading

✅ Deploy Preview for determined-ui canceled.

codecov bot commented Sep 17, 2024 • edited Loading

Codecov Report

hamidzr commented Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gt2345 left a comment

Choose a reason for hiding this comment

stoksc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgongd Oct 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoksc commented Oct 24, 2024 • edited Loading

jgongd commented Sep 17, 2024 •

edited

Loading

netlify bot commented Sep 17, 2024 •

edited

Loading

codecov bot commented Sep 17, 2024 •

edited

Loading

hamidzr commented Sep 30, 2024 •

edited

Loading

jgongd Oct 19, 2024 •

edited

Loading

stoksc commented Oct 24, 2024 •

edited

Loading