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.

Sign up for GitHub

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

Jump to bottom

Backend - Removed hardcoded metrics file name #574

Merged
merged 3 commits into from
Dec 21, 2018
Merged

Backend - Removed hardcoded metrics file name #574

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
017c398
Backend - Removed hardcoded metrics file name
Ark-kun Dec 20, 2018
516cafc
Addressed the PR feedback
Ark-kun Dec 20, 2018
3b58993
Addressed the PR feedback.
Ark-kun Dec 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions backend/src/agent/persistence/worker/metrics_reporter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import (

const (
metricsArtifactName = "mlpipeline-metrics"
metricsJSONFileName = metricsArtifactName + ".json"
// More than 50 metrics is not scalable with current UI design.
maxMetricsCountLimit = 50
)
Expand Down Expand Up @@ -144,11 +143,17 @@ func (r MetricsReporter) readNodeMetricsJSONOrEmpty(runID string, nodeID string)
return "", util.NewCustomError(err, util.CUSTOM_CODE_PERMANENT,
"Unable to extract metrics tgz file read from (%+v): %v", artifactRequest, err)
}
metricsJSON, found := archivedFiles[metricsJSONFileName]
if !found {
return "", nil
//There needs to be exactly one metrics file in the artifact archive. We load that file.
if len(archivedFiles) != 1 {
return "", util.NewCustomErrorf(util.CUSTOM_CODE_PERMANENT,
"There needs to be exactly one metrics file in the artifact archive, but zero or multiple files were found")
}
for key := range archivedFiles { //There must be a single key
metricsJSON := archivedFiles[key]
return metricsJSON, nil
}
return metricsJSON, nil
return "", util.NewCustomErrorf(util.CUSTOM_CODE_PERMANENT,
"There needs to be exactly one metrics file in the artifact archive, but zero files were found")
Ark-kun marked this conversation as resolved.
Show resolved Hide resolved
}

func processReportMetricResults(
Expand Down
86 changes: 81 additions & 5 deletions backend/src/agent/persistence/worker/metrics_reporter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ func TestReportMetrics_Succeed(t *testing.T) {
},
})
metricsJSON := `{"metrics": [{"name": "accuracy", "numberValue": 0.77}, {"name": "logloss", "numberValue": 1.2}]}`
artifactData, _ := util.ArchiveTgz(map[string]string{"mlpipeline-metrics.json": metricsJSON})
artifactData, _ := util.ArchiveTgz(map[string]string{"file": metricsJSON})
pipelineFake.StubArtifact(
&api.ReadArtifactRequest{
RunId: "run-1",
Expand Down Expand Up @@ -130,6 +130,82 @@ func TestReportMetrics_Succeed(t *testing.T) {
assert.Equal(t, expectedMetricsRequest, pipelineFake.GetReportedMetricsRequest())
}

func TestReportMetrics_EmptyArchive_Fail(t *testing.T) {
pipelineFake := client.NewPipelineClientFake()
reporter := NewMetricsReporter(pipelineFake)
workflow := util.NewWorkflow(&workflowapi.Workflow{
ObjectMeta: metav1.ObjectMeta{
Namespace: "MY_NAMESPACE",
Name: "MY_NAME",
UID: types.UID("run-1"),
},
Status: workflowapi.WorkflowStatus{
Nodes: map[string]workflowapi.NodeStatus{
"node-1": workflowapi.NodeStatus{
ID: "node-1",
Phase: workflowapi.NodeSucceeded,
},
},
},
})
artifactData, _ := util.ArchiveTgz(map[string]string{})
pipelineFake.StubArtifact(
&api.ReadArtifactRequest{
RunId: "run-1",
NodeId: "node-1",
ArtifactName: "mlpipeline-metrics",
},
&api.ReadArtifactResponse{
Data: []byte(artifactData),
})

err := reporter.ReportMetrics(workflow)

assert.NotNil(t, err)
assert.True(t, util.HasCustomCode(err, util.CUSTOM_CODE_PERMANENT))
// Verify that ReportRunMetrics is not called.
assert.Nil(t, pipelineFake.GetReportedMetricsRequest())
}

func TestReportMetrics_MultipleFilesInArchive_Fail(t *testing.T) {
pipelineFake := client.NewPipelineClientFake()
reporter := NewMetricsReporter(pipelineFake)
workflow := util.NewWorkflow(&workflowapi.Workflow{
ObjectMeta: metav1.ObjectMeta{
Namespace: "MY_NAMESPACE",
Name: "MY_NAME",
UID: types.UID("run-1"),
},
Status: workflowapi.WorkflowStatus{
Nodes: map[string]workflowapi.NodeStatus{
"node-1": workflowapi.NodeStatus{
ID: "node-1",
Phase: workflowapi.NodeSucceeded,
},
},
},
})
validMetricsJSON := `{"metrics": [{"name": "accuracy", "numberValue": 0.77}, {"name": "logloss", "numberValue": 1.2}]}`
invalidMetricsJSON := `invalid JSON`
artifactData, _ := util.ArchiveTgz(map[string]string{"file1": validMetricsJSON, "file2": invalidMetricsJSON})
pipelineFake.StubArtifact(
&api.ReadArtifactRequest{
RunId: "run-1",
NodeId: "node-1",
ArtifactName: "mlpipeline-metrics",
},
&api.ReadArtifactResponse{
Data: []byte(artifactData),
})

err := reporter.ReportMetrics(workflow)

assert.NotNil(t, err)
assert.True(t, util.HasCustomCode(err, util.CUSTOM_CODE_PERMANENT))
// Verify that ReportRunMetrics is not called.
assert.Nil(t, pipelineFake.GetReportedMetricsRequest())
}

func TestReportMetrics_InvalidMetricsJSON_Fail(t *testing.T) {
pipelineFake := client.NewPipelineClientFake()
reporter := NewMetricsReporter(pipelineFake)
Expand All @@ -149,7 +225,7 @@ func TestReportMetrics_InvalidMetricsJSON_Fail(t *testing.T) {
},
})
metricsJSON := `invalid JSON`
artifactData, _ := util.ArchiveTgz(map[string]string{"mlpipeline-metrics.json": metricsJSON})
artifactData, _ := util.ArchiveTgz(map[string]string{"file": metricsJSON})
pipelineFake.StubArtifact(
&api.ReadArtifactRequest{
RunId: "run-1",
Expand Down Expand Up @@ -192,8 +268,8 @@ func TestReportMetrics_InvalidMetricsJSON_PartialFail(t *testing.T) {
})
validMetricsJSON := `{"metrics": [{"name": "accuracy", "numberValue": 0.77}, {"name": "logloss", "numberValue": 1.2}]}`
invalidMetricsJSON := `invalid JSON`
validArtifactData, _ := util.ArchiveTgz(map[string]string{"mlpipeline-metrics.json": validMetricsJSON})
invalidArtifactData, _ := util.ArchiveTgz(map[string]string{"mlpipeline-metrics.json": invalidMetricsJSON})
validArtifactData, _ := util.ArchiveTgz(map[string]string{"file": validMetricsJSON})
invalidArtifactData, _ := util.ArchiveTgz(map[string]string{"file": invalidMetricsJSON})
// Stub two artifacts, node-1 is invalid, node-2 is valid.
pipelineFake.StubArtifact(
&api.ReadArtifactRequest{
Expand Down Expand Up @@ -293,7 +369,7 @@ func TestReportMetrics_MultiplMetricErrors_TransientErrowWin(t *testing.T) {
})
metricsJSON :=
`{"metrics": [{"name": "accuracy", "numberValue": 0.77}, {"name": "log loss", "numberValue": 1.2}, {"name": "accuracy", "numberValue": 1.2}]}`
artifactData, _ := util.ArchiveTgz(map[string]string{"mlpipeline-metrics.json": metricsJSON})
artifactData, _ := util.ArchiveTgz(map[string]string{"file": metricsJSON})
pipelineFake.StubArtifact(
&api.ReadArtifactRequest{
RunId: "run-1",
Expand Down