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.

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

chore: remove metric from performance #213

Merged
merged 9 commits into from
May 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

- A Performance in now unique regarding a compute task key, a metric key and a compute task output identifier ([#197](https://github.com/Substra/orchestrator/pull/197))

### Removed

- Metric from Performance ([#213](https://github.com/Substra/orchestrator/pull/213))

## [0.33.0](https://github.com/Substra/orchestrator/releases/tag/0.33.0) - 2023-03-31

### Changed
Expand Down
5 changes: 1 addition & 4 deletions chaincode/ledger/dbal_performance.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func (db *DB) AddPerformance(perf *asset.Performance, identifier string) error {
return err
}

return db.createIndex(performanceIndex, []string{asset.PerformanceKind, perf.GetComputeTaskKey(), perf.GetMetricKey(), perf.GetComputeTaskOutputIdentifier()})
return db.createIndex(performanceIndex, []string{asset.PerformanceKind, perf.GetComputeTaskKey(), perf.GetComputeTaskOutputIdentifier()})
}

// PerformanceExists implements persistence.PerformanceDBAL
Expand All @@ -47,9 +47,6 @@ func (db *DB) QueryPerformances(p *common.Pagination, filter *asset.PerformanceQ
if filter.ComputeTaskKey != "" {
assetFilter["compute_task_key"] = filter.ComputeTaskKey
}
if filter.MetricKey != "" {
assetFilter["metric_key"] = filter.MetricKey
}
if filter.ComputeTaskOutputIdentifier != "" {
assetFilter["compute_task_output_identifier"] = filter.ComputeTaskOutputIdentifier
}
Expand Down
1,450 changes: 720 additions & 730 deletions docs/schemas/standalone-database.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 0 additions & 1 deletion e2e/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,6 @@ func (c *TestClient) RegisterPerformance(o *PerformanceOptions) (*asset.Performa
newPerf := &asset.NewPerformance{
ComputeTaskKey: c.ks.GetKey(o.ComputeTaskKeyRef),
ComputeTaskOutputIdentifier: o.ComputeTaskOutput,
MetricKey: c.ks.GetKey(o.MetricKeyRef),
PerformanceValue: o.PerformanceValue,
}

Expand Down
5 changes: 0 additions & 5 deletions e2e/client/option.go
Original file line number Diff line number Diff line change
Expand Up @@ -490,11 +490,6 @@ func (o *PerformanceOptions) WithTaskOutput(output string) *PerformanceOptions {
return o
}

func (o *PerformanceOptions) WithMetricRef(ref string) *PerformanceOptions {
o.MetricKeyRef = ref
return o
}

func DefaultDataSampleOptions() *DataSampleOptions {
return &DataSampleOptions{
KeyRef: "ds",
Expand Down
12 changes: 6 additions & 6 deletions e2e/performance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func TestRegisterPerformance(t *testing.T) {
WithInput("predictions", &client.TaskOutputRef{TaskRef: "predictTask", Identifier: "predictions"}))
appClient.StartTask("testTask")

registeredPerf, err := appClient.RegisterPerformance(client.DefaultPerformanceOptions().WithTaskRef("testTask").WithMetricRef("testmetric").WithTaskOutput("performance"))
registeredPerf, err := appClient.RegisterPerformance(client.DefaultPerformanceOptions().WithTaskRef("testTask").WithTaskOutput("performance"))
require.NoError(t, err)

appClient.DoneTask("testTask")
Expand Down Expand Up @@ -102,15 +102,15 @@ func TestRegisterMultiplePerformances(t *testing.T) {
WithFunctionRef("testmetric"))
appClient.StartTask("testTask")

_, err := appClient.RegisterPerformance(client.DefaultPerformanceOptions().WithTaskRef("testTask").WithMetricRef("testmetric").WithTaskOutput("performance"))
_, err := appClient.RegisterPerformance(client.DefaultPerformanceOptions().WithTaskRef("testTask").WithTaskOutput("performance"))
require.NoError(t, err)
appClient.DoneTask("testTask")

task := appClient.GetComputeTask("testTask")
require.Equal(t, asset.ComputeTaskStatus_STATUS_DONE, task.Status)
}

func TestRegisterMultiplePerformancesForSameMetric(t *testing.T) {
func TestRegisterMultiplePerformancesForSameTaskOutput(t *testing.T) {
appClient := factory.NewTestClient()

appClient.RegisterFunction(client.DefaultSimpleFunctionOptions())
Expand Down Expand Up @@ -141,14 +141,14 @@ func TestRegisterMultiplePerformancesForSameMetric(t *testing.T) {
WithInput("predictions", &client.TaskOutputRef{TaskRef: "predictTask", Identifier: "predictions"}))
appClient.StartTask("testTask")

_, err := appClient.RegisterPerformance(client.DefaultPerformanceOptions().WithTaskRef("testTask").WithMetricRef("testmetric").WithTaskOutput("performance"))
_, err := appClient.RegisterPerformance(client.DefaultPerformanceOptions().WithTaskRef("testTask").WithTaskOutput("performance"))
require.NoError(t, err)

appClient.DoneTask("testTask")
task := appClient.GetComputeTask("testTask")
require.Equal(t, asset.ComputeTaskStatus_STATUS_DONE, task.Status)

_, err = appClient.RegisterPerformance(client.DefaultPerformanceOptions().WithTaskRef("testTask").WithMetricRef("testmetric").WithTaskOutput("performance"))
_, err = appClient.RegisterPerformance(client.DefaultPerformanceOptions().WithTaskRef("testTask").WithTaskOutput("performance"))
require.ErrorContains(t, err, orcerrors.ErrBadRequest)

task = appClient.GetComputeTask("testTask")
Expand Down Expand Up @@ -187,7 +187,7 @@ func TestQueryPerformances(t *testing.T) {
appClient.StartTask("testTask")

_, err := appClient.RegisterPerformance(
client.DefaultPerformanceOptions().WithTaskRef("testTask").WithMetricRef("testmetric").WithTaskOutput("performance"),
client.DefaultPerformanceOptions().WithTaskRef("testTask").WithTaskOutput("performance"),
)
require.NoError(t, err)

Expand Down
4 changes: 2 additions & 2 deletions lib/asset/performance.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package asset

import "strings"

// GetKey returns the performance key derived from its referenced task, metric and identifier.
// GetKey returns the performance key derived from its referenced task and identifier.
func (p *Performance) GetKey() string {
return strings.Join([]string{p.ComputeTaskKey, p.MetricKey, p.ComputeTaskOutputIdentifier}, "|")
return strings.Join([]string{p.ComputeTaskKey, p.ComputeTaskOutputIdentifier}, "|")
}
3 changes: 0 additions & 3 deletions lib/asset/performance.proto
Original file line number Diff line number Diff line change
Expand Up @@ -9,21 +9,18 @@ import "google/protobuf/timestamp.proto";
message NewPerformance {
string compute_task_key = 1;
string compute_task_output_identifier = 3;
string metric_key = 5;
float performance_value = 2;
}

message Performance {
string compute_task_key = 1;
string metric_key = 5;
string compute_task_output_identifier = 6;
float performance_value = 2;
google.protobuf.Timestamp creation_date = 3;
}

message PerformanceQueryFilter {
string compute_task_key = 1;
string metric_key = 2;
string compute_task_output_identifier = 3;
}

Expand Down
3 changes: 1 addition & 2 deletions lib/asset/performance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,7 @@ import (
func TestPerformanceGetKey(t *testing.T) {
perf := &Performance{
ComputeTaskKey: "taskKey",
MetricKey: "metricKey",
ComputeTaskOutputIdentifier: "performance",
}
assert.Equal(t, perf.GetKey(), "taskKey|metricKey|performance")
assert.Equal(t, perf.GetKey(), "taskKey|performance")
}
1 change: 0 additions & 1 deletion lib/asset/performance_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,5 @@ func (a *NewPerformance) Validate() error {
return validation.ValidateStruct(a,
validation.Field(&a.ComputeTaskKey, validation.Required, is.UUID),
validation.Field(&a.ComputeTaskOutputIdentifier, validation.Required),
validation.Field(&a.MetricKey, validation.Required, is.UUID),
)
}
9 changes: 0 additions & 9 deletions lib/asset/performance_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,24 +17,15 @@ func TestPerformanceValidate(t *testing.T) {
"invalidComputeTaskKey": {&NewPerformance{
ComputeTaskKey: "not36chars",
ComputeTaskOutputIdentifier: "auc",
MetricKey: "1da600d4-f8ad-45d7-92a0-7ff752a82275",
PerformanceValue: 0.5,
}, false},
"invalidMetricKey": {&NewPerformance{
ComputeTaskKey: "08680966-97ae-4573-8b2d-6c4db2b3c532",
ComputeTaskOutputIdentifier: "auc",
MetricKey: "not36chars",
PerformanceValue: 0.5,
}, false},
"missingComputeTaskOutput": {&NewPerformance{
ComputeTaskKey: "08680966-97ae-4573-8b2d-6c4db2b3c532",
MetricKey: "not36chars",
PerformanceValue: 0.5,
}, false},
"valid": {&NewPerformance{
ComputeTaskKey: "08680966-97ae-4573-8b2d-6c4db2b3c532",
ComputeTaskOutputIdentifier: "auc",
MetricKey: "1da600d4-f8ad-45d7-92a0-7ff752a82275",
PerformanceValue: 0.5,
}, true},
}
Expand Down
20 changes: 0 additions & 20 deletions lib/service/performance.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ func NewPerformanceService(provider PerformanceDependencyProvider) *PerformanceS
func (s *PerformanceService) RegisterPerformance(newPerf *asset.NewPerformance, requester string) (*asset.Performance, error) {
s.GetLogger().Debug().
Str("taskKey", newPerf.ComputeTaskKey).
Str("metricKey", newPerf.MetricKey).
Str("computeTaskOutputIdentifier", newPerf.ComputeTaskOutputIdentifier).
Str("requester", requester).
Msg("Registering new performance")
Expand All @@ -63,33 +62,14 @@ func (s *PerformanceService) RegisterPerformance(newPerf *asset.NewPerformance,
return nil, errors.NewBadRequest(fmt.Sprintf("cannot register performance for task with status %q", task.Status.String()))
}

if newPerf.MetricKey != task.FunctionKey {
return nil, errors.NewBadRequest(fmt.Sprintf("Function used for metric with key %s should be the same than the one in task with key %s", newPerf.MetricKey, task.FunctionKey))
}

functionPerf, err := s.GetFunctionService().GetFunction(newPerf.MetricKey)
if err != nil {
return nil, err
}

if _, ok := task.Outputs[newPerf.ComputeTaskOutputIdentifier]; !ok {
return nil, errors.NewMissingTaskOutput(task.Key, newPerf.ComputeTaskOutputIdentifier)
}

functionOutput, ok := functionPerf.Outputs[newPerf.ComputeTaskOutputIdentifier]
if !ok {
// This should never happen since task outputs are checked against function on registration
return nil, errors.NewInternal(fmt.Sprintf("missing function output %q for task %q", newPerf.ComputeTaskOutputIdentifier, task.Key))
}
if functionOutput.Kind != asset.AssetKind_ASSET_PERFORMANCE {
return nil, errors.NewIncompatibleTaskOutput(task.Key, newPerf.ComputeTaskOutputIdentifier, functionOutput.Kind.String(), asset.AssetKind_ASSET_PERFORMANCE.String())
}

perf := &asset.Performance{
ComputeTaskKey: newPerf.ComputeTaskKey,
PerformanceValue: newPerf.PerformanceValue,
CreationDate: timestamppb.New(s.GetTimeService().GetTransactionTime()),
MetricKey: newPerf.MetricKey,
ComputeTaskOutputIdentifier: newPerf.ComputeTaskOutputIdentifier,
}

Expand Down
69 changes: 1 addition & 68 deletions lib/service/performance_test.go
Original file line number Diff line number Diff line change
@@ -1,63 +1,48 @@
package service

import (
"errors"
"testing"
"time"

"github.com/stretchr/testify/assert"
"github.com/substra/orchestrator/lib/asset"
orcerrors "github.com/substra/orchestrator/lib/errors"
"github.com/substra/orchestrator/lib/persistence"
"google.golang.org/protobuf/types/known/timestamppb"
)

func TestRegisterPerformance(t *testing.T) {
dbal := new(persistence.MockDBAL)
as := new(MockFunctionAPI)
cts := new(MockComputeTaskAPI)
es := new(MockEventAPI)
ts := new(MockTimeAPI)
provider := newMockedProvider()
provider.On("GetComputeTaskService").Return(cts)
provider.On("GetFunctionService").Return(as)
provider.On("GetPerformanceDBAL").Return(dbal)
provider.On("GetEventService").Return(es)
provider.On("GetTimeService").Return(ts)
service := NewPerformanceService(provider)

ts.On("GetTransactionTime").Once().Return(time.Unix(1337, 0))

metric := &asset.Function{
Key: "1da600d4-f8ad-45d7-92a0-7ff752a82275",
Outputs: map[string]*asset.FunctionOutput{
"auc": {
Kind: asset.AssetKind_ASSET_PERFORMANCE,
},
}}
as.On("GetFunction", "1da600d4-f8ad-45d7-92a0-7ff752a82275").Return(metric, nil)

task := &asset.ComputeTask{
Key: "taskTest",
Status: asset.ComputeTaskStatus_STATUS_DOING,
Worker: "test",
Outputs: map[string]*asset.ComputeTaskOutput{
"auc": {},
},
FunctionKey: metric.Key,
FunctionKey: "1da600d4-f8ad-45d7-92a0-7ff752a82275",
}
cts.On("GetTask", "08680966-97ae-4573-8b2d-6c4db2b3c532").Return(task, nil)

perf := &asset.NewPerformance{
ComputeTaskKey: "08680966-97ae-4573-8b2d-6c4db2b3c532",
ComputeTaskOutputIdentifier: "auc",
MetricKey: "1da600d4-f8ad-45d7-92a0-7ff752a82275",
PerformanceValue: 0.36492,
}

stored := &asset.Performance{
ComputeTaskKey: perf.ComputeTaskKey,
MetricKey: perf.MetricKey,
ComputeTaskOutputIdentifier: perf.ComputeTaskOutputIdentifier,
PerformanceValue: perf.PerformanceValue,
CreationDate: timestamppb.New(time.Unix(1337, 0)),
Expand Down Expand Up @@ -87,7 +72,6 @@ func TestRegisterPerformance(t *testing.T) {
_, err := service.RegisterPerformance(perf, "test")
assert.NoError(t, err)

as.AssertExpectations(t)
cts.AssertExpectations(t)
provider.AssertExpectations(t)
ts.AssertExpectations(t)
Expand All @@ -107,7 +91,6 @@ func TestRegisterPerformanceInvalidTask(t *testing.T) {
perf := &asset.NewPerformance{
ComputeTaskKey: "08680966-97ae-4573-8b2d-6c4db2b3c532",
ComputeTaskOutputIdentifier: "auc",
MetricKey: "1da600d4-f8ad-45d7-92a0-7ff752a82275",
PerformanceValue: 0.36492,
}

Expand All @@ -117,53 +100,3 @@ func TestRegisterPerformanceInvalidTask(t *testing.T) {
cts.AssertExpectations(t)
provider.AssertExpectations(t)
}

func TestRegisterPerformanceInvalidOutput(t *testing.T) {
as := new(MockFunctionAPI)
cts := new(MockComputeTaskAPI)
provider := newMockedProvider()
provider.On("GetComputeTaskService").Return(cts)
provider.On("GetFunctionService").Return(as)
service := NewPerformanceService(provider)

metric := &asset.Function{
Key: "1da600d4-f8ad-45d7-92a0-7ff752a82275",
Outputs: map[string]*asset.FunctionOutput{
"auc": {
Kind: asset.AssetKind_ASSET_UNKNOWN,
},
}}
as.On("GetFunction", "1da600d4-f8ad-45d7-92a0-7ff752a82275").Return(metric, nil)

task := &asset.ComputeTask{
Status: asset.ComputeTaskStatus_STATUS_DOING,
Worker: "test",
Outputs: map[string]*asset.ComputeTaskOutput{
"auc": {},
},
FunctionKey: metric.Key,
}
cts.On("GetTask", "08680966-97ae-4573-8b2d-6c4db2b3c532").Return(task, nil)

perf := &asset.NewPerformance{
ComputeTaskKey: "08680966-97ae-4573-8b2d-6c4db2b3c532",
ComputeTaskOutputIdentifier: "foo",
MetricKey: metric.Key,
PerformanceValue: 0.36492,
}

_, err := service.RegisterPerformance(perf, "test")
assert.ErrorContains(t, err, "has no output named \"foo\"")
orcError := new(orcerrors.OrcError)
assert.True(t, errors.As(err, &orcError))
assert.Equal(t, orcerrors.ErrMissingTaskOutput, orcError.Kind)

perf.ComputeTaskOutputIdentifier = "auc"
_, err = service.RegisterPerformance(perf, "test")
orcError = new(orcerrors.OrcError)
assert.True(t, errors.As(err, &orcError))
assert.Equal(t, orcerrors.ErrIncompatibleKind, orcError.Kind)
as.AssertExpectations(t)
cts.AssertExpectations(t)
provider.AssertExpectations(t)
}
4 changes: 2 additions & 2 deletions server/distributed/adapters/performance.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ func (a *PerformanceAdapter) RegisterPerformance(ctx context.Context, newPerf *a
PageToken: "",
PageSize: 1,
Filter: &asset.PerformanceQueryFilter{
ComputeTaskKey: newPerf.ComputeTaskKey,
MetricKey: newPerf.MetricKey,
ComputeTaskKey: newPerf.ComputeTaskKey,
ComputeTaskOutputIdentifier: newPerf.ComputeTaskOutputIdentifier,
},
},
response,
Expand Down
12 changes: 6 additions & 6 deletions server/distributed/adapters/performance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ func TestHandlePerfConflictAfterTimeout(t *testing.T) {
invocator := &chaincode.MockInvocator{}

newPerf := &asset.NewPerformance{
ComputeTaskKey: "taskUuid",
MetricKey: "metricUuid",
ComputeTaskKey: "taskUuid",
ComputeTaskOutputIdentifier: "my_perf",
}

// register fail
Expand All @@ -78,8 +78,8 @@ func TestHandlePerfConflictAfterTimeout(t *testing.T) {
PageToken: "",
PageSize: 1,
Filter: &asset.PerformanceQueryFilter{
ComputeTaskKey: newPerf.ComputeTaskKey,
MetricKey: newPerf.MetricKey,
ComputeTaskKey: newPerf.ComputeTaskKey,
ComputeTaskOutputIdentifier: newPerf.ComputeTaskOutputIdentifier,
},
}
invocator.On(
Expand All @@ -92,8 +92,8 @@ func TestHandlePerfConflictAfterTimeout(t *testing.T) {
response := args.Get(3).(*asset.QueryPerformancesResponse)
response.Performances = []*asset.Performance{
{
ComputeTaskKey: newPerf.ComputeTaskKey,
MetricKey: newPerf.MetricKey,
ComputeTaskKey: newPerf.ComputeTaskKey,
ComputeTaskOutputIdentifier: newPerf.ComputeTaskOutputIdentifier,
},
}
}).Return(nil)
Expand Down
Loading