From 3dc9e2b1373289b8934657fc26691e08a4136958 Mon Sep 17 00:00:00 2001 From: Gbolahan <89295500+galadd@users.noreply.github.com> Date: Tue, 22 Aug 2023 22:28:02 +0100 Subject: [PATCH] Delete first run in AimUI (#195) * fix: delete first run in aim ui (#158) * Add integration test to delete first run - row 0 * Change Errorf to New * Add unit test to test for negative row number in renumberRows * gofmt run_test * Add cases for (-1,0,1) * Use proper format for test --------- Co-authored-by: Geoffrey Wilson Co-authored-by: Software Developer <7852635+dsuhinin@users.noreply.github.com> --- go.mod | 1 + go.sum | 2 + pkg/api/mlflow/dao/repositories/experiment.go | 2 +- pkg/api/mlflow/dao/repositories/run.go | 6 +- pkg/api/mlflow/dao/repositories/run_test.go | 64 +++++++++++++++++++ .../integration/golang/aim/run/delete_test.go | 5 ++ 6 files changed, 76 insertions(+), 4 deletions(-) create mode 100644 pkg/api/mlflow/dao/repositories/run_test.go diff --git a/go.mod b/go.mod index 7d2781256..1395af677 100644 --- a/go.mod +++ b/go.mod @@ -94,6 +94,7 @@ require ( golang.org/x/text v0.9.0 // indirect golang.org/x/tools v0.9.1 // indirect golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2 // indirect + gopkg.in/DATA-DOG/go-sqlmock.v1 v1.3.0 // indirect gopkg.in/ini.v1 v1.67.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect ) diff --git a/go.sum b/go.sum index ecadcf6de..8b585d0fa 100644 --- a/go.sum +++ b/go.sum @@ -616,6 +616,8 @@ google.golang.org/protobuf v1.23.0/go.mod h1:EGpADcykh3NcUnDUJcl1+ZksZNG86OlYog2 google.golang.org/protobuf v1.23.1-0.20200526195155-81db48ad09cc/go.mod h1:EGpADcykh3NcUnDUJcl1+ZksZNG86OlYog2l/sGQquU= google.golang.org/protobuf v1.24.0/go.mod h1:r/3tXBNzIEhYS9I1OUVjXDlt8tc493IdKGjtUeSXeh4= google.golang.org/protobuf v1.25.0/go.mod h1:9JNX74DMeImyA3h4bdi1ymwjUzf21/xIlbajtzgsN7c= +gopkg.in/DATA-DOG/go-sqlmock.v1 v1.3.0 h1:FVCohIoYO7IJoDDVpV2pdq7SgrMH6wHnuTyrdrxJNoY= +gopkg.in/DATA-DOG/go-sqlmock.v1 v1.3.0/go.mod h1:OdE7CF6DbADk7lN8LIKRzRJTTZXIjtWgA5THM5lhBAw= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk= diff --git a/pkg/api/mlflow/dao/repositories/experiment.go b/pkg/api/mlflow/dao/repositories/experiment.go index 0b06dedca..e27907072 100644 --- a/pkg/api/mlflow/dao/repositories/experiment.go +++ b/pkg/api/mlflow/dao/repositories/experiment.go @@ -135,7 +135,7 @@ func (r ExperimentRepository) DeleteBatch(ctx context.Context, ids []*int32) err // verify deletion if len(experiments) != len(ids) { - return eris.Errorf("count of deleted experiments does not match length of ids input (invalid experiment ID?)") + return eris.New("count of deleted experiments does not match length of ids input (invalid experiment ID?)") } // renumbering the remainder runs diff --git a/pkg/api/mlflow/dao/repositories/run.go b/pkg/api/mlflow/dao/repositories/run.go index a7e881131..1ffcdc4d7 100644 --- a/pkg/api/mlflow/dao/repositories/run.go +++ b/pkg/api/mlflow/dao/repositories/run.go @@ -172,7 +172,7 @@ func (r RunRepository) DeleteBatch(ctx context.Context, ids []string) error { // verify deletion // NOTE: tx.RowsAffected does not provide correct number of deleted, using the returning slice instead if len(runs) != len(ids) { - return eris.Errorf("count of deleted runs does not match length of ids input (invalid run ID?)") + return eris.New("count of deleted runs does not match length of ids input (invalid run ID?)") } // renumber the remainder @@ -266,8 +266,8 @@ func getMinRowNum(runs []models.Run) models.RowNum { // renumberRows will update the runs.row_num field with the correct ordinal func (r RunRepository) renumberRows(tx *gorm.DB, startWith models.RowNum) error { - if startWith <= models.RowNum(0) { - return eris.Errorf("attempting to renumber with 0 or less row number value") + if startWith < models.RowNum(0) { + return eris.New("attempting to renumber with less than 0 row number value") } if tx.Dialector.Name() == "postgres" { diff --git a/pkg/api/mlflow/dao/repositories/run_test.go b/pkg/api/mlflow/dao/repositories/run_test.go new file mode 100644 index 000000000..ce7e5e63a --- /dev/null +++ b/pkg/api/mlflow/dao/repositories/run_test.go @@ -0,0 +1,64 @@ +package repositories + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "gopkg.in/DATA-DOG/go-sqlmock.v1" + "gorm.io/driver/postgres" + "gorm.io/gorm" + + "github.com/G-Research/fasttrackml/pkg/api/mlflow/dao/models" +) + +func Test_renumberRows(t *testing.T) { + testData := []struct { + name string + startWith models.RowNum + }{ + { + name: "NegativeRowNumber", + startWith: models.RowNum(-1), + }, + { + name: "ZeroRowNumber", + startWith: models.RowNum(0), + }, + { + name: "PositiveRowNumber", + startWith: models.RowNum(1), + }, + } + + mockDb, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("an error '%s' was not expected when opening a stub database connection", err) + } + defer mockDb.Close() + + lockExpect := func() { + mock.ExpectExec("LOCK TABLE runs").WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectExec(`UPDATE runs`).WillReturnResult(sqlmock.NewResult(0, 1)) + } + + dialector := postgres.New(postgres.Config{ + Conn: mockDb, + DriverName: "postgres", + }) + db, _ := gorm.Open(dialector, &gorm.Config{}) + + repo := NewRunRepository(db) + + for _, tc := range testData { + t.Run(tc.name, func(t *testing.T) { + if tc.startWith < 0 { + err := repo.renumberRows(db, tc.startWith) + assert.EqualError(t, err, "attempting to renumber with less than 0 row number value") + } else { + lockExpect() + err := repo.renumberRows(db, tc.startWith) + assert.NoError(t, err) + } + }) + } +} diff --git a/tests/integration/golang/aim/run/delete_test.go b/tests/integration/golang/aim/run/delete_test.go index 14abae03e..db4b2e30e 100644 --- a/tests/integration/golang/aim/run/delete_test.go +++ b/tests/integration/golang/aim/run/delete_test.go @@ -72,6 +72,11 @@ func (s *DeleteRunTestSuite) Test_Ok() { request: request.DeleteRunRequest{RunID: s.runs[1].ID}, expectedRunCount: 8, }, + { + name: "DeleteFirstRun", + request: request.DeleteRunRequest{RunID: s.runs[0].ID}, + expectedRunCount: 7, + }, } for _, tt := range tests { s.T().Run(tt.name, func(T *testing.T) {