Repository: G-Research/fasttrackml
Pull request #659 (2023-11-28 10:58:55)

Test NaN metrics values.
What's inside the box:
- increase `integration` tests coverage for `NaN`, `+Infinite`, `-Infinite` values.


Patch:
diff --git a/pkg/api/mlflow/dao/convertors/log.go b/pkg/api/mlflow/dao/convertors/log.go
index 3e6e4d6..07e5751 100644
--- a/pkg/api/mlflow/dao/convertors/log.go
+++ b/pkg/api/mlflow/dao/convertors/log.go
@@ -66,7 +66,7 @@ func ConvertLogBatchRequestToDBModel(
 				return nil, nil, nil, eris.Errorf("invalid metric value '%s'", v)
 			}
 		} else {
-			return nil, nil, nil, eris.Errorf("invalid metric value '%s'", v)
+			return nil, nil, nil, eris.Errorf("invalid metric value '%v'", metric.Value)
 		}
 		metrics[n] = m
 	}
diff --git a/pkg/api/mlflow/dao/convertors/metric.go b/pkg/api/mlflow/dao/convertors/metric.go
index af0cf4f..4d02761 100644
--- a/pkg/api/mlflow/dao/convertors/metric.go
+++ b/pkg/api/mlflow/dao/convertors/metric.go
@@ -10,8 +10,8 @@ import (
 	"github.com/G-Research/fasttrackml/pkg/api/mlflow/dao/models"
 )
 
-// ConvertMetricParamRequestToDBModel converts request.LogMetricRequest into actual models.Metric model.
-func ConvertMetricParamRequestToDBModel(runID string, req *request.LogMetricRequest) (*models.Metric, error) {
+// ConvertLogMetricRequestToDBModel converts request.LogMetricRequest into actual models.Metric model.
+func ConvertLogMetricRequestToDBModel(runID string, req *request.LogMetricRequest) (*models.Metric, error) {
 	metric := models.Metric{
 		Key:       req.Key,
 		Timestamp: req.Timestamp,
@@ -33,7 +33,7 @@ func ConvertMetricParamRequestToDBModel(runID string, req *request.LogMetricRequ
 			return nil, eris.Errorf("invalid metric value '%s'", v)
 		}
 	} else {
-		return nil, eris.Errorf("invalid metric value '%s'", v)
+		return nil, eris.Errorf("invalid metric value '%v'", req.Value)
 	}
 	return &metric, nil
 }
diff --git a/pkg/api/mlflow/service/run/service.go b/pkg/api/mlflow/service/run/service.go
index fbe9453..9a90133 100644
--- a/pkg/api/mlflow/service/run/service.go
+++ b/pkg/api/mlflow/service/run/service.go
@@ -491,7 +491,7 @@ func (s Service) LogMetric(
 		return api.NewResourceDoesNotExistError("unable to find run '%s'", req.RunID)
 	}
 
-	metric, err := convertors.ConvertMetricParamRequestToDBModel(run.ID, req)
+	metric, err := convertors.ConvertLogMetricRequestToDBModel(run.ID, req)
 	if err != nil {
 		return api.NewInvalidParameterValueError(err.Error())
 	}


Test patch:
diff --git a/pkg/api/mlflow/dao/convertors/log_test.go b/pkg/api/mlflow/dao/convertors/log_test.go
index 5da7efb..6676336 100644
--- a/pkg/api/mlflow/dao/convertors/log_test.go
+++ b/pkg/api/mlflow/dao/convertors/log_test.go
@@ -1,6 +1,8 @@
 package convertors
 
 import (
+	"errors"
+	"math"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -22,49 +24,271 @@ func TestConvertLogParamRequestToDBModel_Ok(t *testing.T) {
 }
 
 func TestConvertLogBatchRequestToDBModel_Ok(t *testing.T) {
-	req := request.LogBatchRequest{
-		Tags: []request.TagPartialRequest{{
-			Key:   "key",
-			Value: "value",
-		}},
-		Params: []request.ParamPartialRequest{
-			{
-				Key:   "key",
-				Value: "value",
+	testData := []struct {
+		name            string
+		request         *request.LogBatchRequest
+		expectedTags    []models.Tag
+		expectedParams  []models.Param
+		expectedMetrics []models.Metric
+	}{
+		{
+			name: "WithMetricNormalValue",
+			request: &request.LogBatchRequest{
+				Tags: []request.TagPartialRequest{{
+					Key:   "key",
+					Value: "value",
+				}},
+				Params: []request.ParamPartialRequest{
+					{
+						Key:   "key",
+						Value: "value",
+					},
+				},
+				Metrics: []request.MetricPartialRequest{
+					{
+						Key:       "key",
+						Value:     1.1,
+						Timestamp: 1234567890,
+						Step:      1,
+					},
+				},
+			},
+			expectedTags: []models.Tag{
+				{
+					RunID: "run_id",
+					Key:   "key",
+					Value: "value",
+				},
+			},
+			expectedParams: []models.Param{
+				{
+					RunID: "run_id",
+					Key:   "key",
+					Value: "value",
+				},
+			},
+			expectedMetrics: []models.Metric{
+				{
+					Key:       "key",
+					Value:     1.1,
+					Timestamp: 1234567890,
+					RunID:     "run_id",
+					Step:      1,
+				},
 			},
 		},
-		Metrics: []request.MetricPartialRequest{
-			{
-				Key:       "key",
-				Value:     1.1,
-				Timestamp: 1234567890,
-				Step:      1,
+		{
+			name: "WithMetricNaNValue",
+			request: &request.LogBatchRequest{
+				Tags: []request.TagPartialRequest{{
+					Key:   "key",
+					Value: "value",
+				}},
+				Params: []request.ParamPartialRequest{
+					{
+						Key:   "key",
+						Value: "value",
+					},
+				},
+				Metrics: []request.MetricPartialRequest{
+					{
+						Key:       "key",
+						Value:     "NaN",
+						Timestamp: 1234567890,
+						Step:      1,
+					},
+				},
+			},
+			expectedTags: []models.Tag{
+				{
+					RunID: "run_id",
+					Key:   "key",
+					Value: "value",
+				},
+			},
+			expectedParams: []models.Param{
+				{
+					RunID: "run_id",
+					Key:   "key",
+					Value: "value",
+				},
+			},
+			expectedMetrics: []models.Metric{
+				{
+					Key:       "key",
+					Value:     0,
+					IsNan:     true,
+					Timestamp: 1234567890,
+					RunID:     "run_id",
+					Step:      1,
+				},
 			},
 		},
-	}
-	metrics, params, tags, err := ConvertLogBatchRequestToDBModel("run_id", &req)
-	require.Nil(t, err)
-	assert.Equal(t, []models.Tag{
 		{
-			RunID: "run_id",
-			Key:   "key",
-			Value: "value",
+			name: "WithMetricPositiveInfinityValue",
+			request: &request.LogBatchRequest{
+				Tags: []request.TagPartialRequest{{
+					Key:   "key",
+					Value: "value",
+				}},
+				Params: []request.ParamPartialRequest{
+					{
+						Key:   "key",
+						Value: "value",
+					},
+				},
+				Metrics: []request.MetricPartialRequest{
+					{
+						Key:       "key",
+						Value:     "Infinity",
+						Timestamp: 1234567890,
+						Step:      1,
+					},
+				},
+			},
+			expectedTags: []models.Tag{
+				{
+					RunID: "run_id",
+					Key:   "key",
+					Value: "value",
+				},
+			},
+			expectedParams: []models.Param{
+				{
+					RunID: "run_id",
+					Key:   "key",
+					Value: "value",
+				},
+			},
+			expectedMetrics: []models.Metric{
+				{
+					Key:       "key",
+					Value:     math.MaxFloat64,
+					Timestamp: 1234567890,
+					RunID:     "run_id",
+					Step:      1,
+				},
+			},
 		},
-	}, tags)
-	assert.Equal(t, []models.Param{
 		{
-			RunID: "run_id",
-			Key:   "key",
-			Value: "value",
+			name: "WithMetricNegativeInfinityValue",
+			request: &request.LogBatchRequest{
+				Tags: []request.TagPartialRequest{{
+					Key:   "key",
+					Value: "value",
+				}},
+				Params: []request.ParamPartialRequest{
+					{
+						Key:   "key",
+						Value: "value",
+					},
+				},
+				Metrics: []request.MetricPartialRequest{
+					{
+						Key:       "key",
+						Value:     "-Infinity",
+						Timestamp: 1234567890,
+						Step:      1,
+					},
+				},
+			},
+			expectedTags: []models.Tag{
+				{
+					RunID: "run_id",
+					Key:   "key",
+					Value: "value",
+				},
+			},
+			expectedParams: []models.Param{
+				{
+					RunID: "run_id",
+					Key:   "key",
+					Value: "value",
+				},
+			},
+			expectedMetrics: []models.Metric{
+				{
+					Key:       "key",
+					Value:     -math.MaxFloat64,
+					Timestamp: 1234567890,
+					RunID:     "run_id",
+					Step:      1,
+				},
+			},
+		},
+	}
+
+	for _, tt := range testData {
+		t.Run(tt.name, func(t *testing.T) {
+			metrics, params, tags, err := ConvertLogBatchRequestToDBModel("run_id", tt.request)
+			require.Nil(t, err)
+			assert.Equal(t, tt.expectedTags, tags)
+			assert.Equal(t, tt.expectedParams, params)
+			assert.Equal(t, tt.expectedMetrics, metrics)
+		})
+	}
+}
+
+func TestConvertLogBatchRequestToDBModel_Error(t *testing.T) {
+	testData := []struct {
+		name    string
+		request *request.LogBatchRequest
+		error   error
+	}{
+		{
+			name: "WithUnsupportedMetricValue",
+			request: &request.LogBatchRequest{
+				Tags: []request.TagPartialRequest{{
+					Key:   "key",
+					Value: "value",
+				}},
+				Params: []request.ParamPartialRequest{
+					{
+						Key:   "key",
+						Value: "value",
+					},
+				},
+				Metrics: []request.MetricPartialRequest{
+					{
+						Key:       "key",
+						Value:     struct{}{},
+						Timestamp: 1234567890,
+						Step:      1,
+					},
+				},
+			},
+			error: errors.New("invalid metric value '{}'"),
 		},
-	}, params)
-	assert.Equal(t, []models.Metric{
 		{
-			Key:       "key",
-			Value:     1.1,
-			Timestamp: 1234567890,
-			RunID:     "run_id",
-			Step:      1,
+			name: "WithUnsupportedNaNMetricValue",
+			request: &request.LogBatchRequest{
+				Tags: []request.TagPartialRequest{{
+					Key:   "key",
+					Value: "value",
+				}},
+				Params: []request.ParamPartialRequest{
+					{
+						Key:   "key",
+						Value: "value",
+					},
+				},
+				Metrics: []request.MetricPartialRequest{
+					{
+						Key:       "key",
+						Value:     "UnsupportedNaNValue",
+						Timestamp: 1234567890,
+						Step:      1,
+					},
+				},
+			},
+			error: errors.New("invalid metric value 'UnsupportedNaNValue'"),
 		},
-	}, metrics)
+	}
+
+	for _, tt := range testData {
+		t.Run(tt.name, func(t *testing.T) {
+			_, _, _, err := ConvertLogBatchRequestToDBModel("run_id", tt.request)
+			assert.Equal(t, tt.error.Error(), err.Error())
+		})
+	}
 }
diff --git a/pkg/api/mlflow/dao/convertors/metric_test.go b/pkg/api/mlflow/dao/convertors/metric_test.go
index abccc3f..1054222 100644
--- a/pkg/api/mlflow/dao/convertors/metric_test.go
+++ b/pkg/api/mlflow/dao/convertors/metric_test.go
@@ -1,27 +1,137 @@
 package convertors
 
 import (
+	"errors"
+	"math"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/G-Research/fasttrackml/pkg/api/mlflow/api/request"
+	"github.com/G-Research/fasttrackml/pkg/api/mlflow/dao/models"
 )
 
-func TestConvertMetricParamRequestToDBModel(t *testing.T) {
-	req := request.LogMetricRequest{
-		Key:       "key",
-		Step:      1,
-		Value:     1.1,
-		RunID:     "run_id",
-		Timestamp: 1234567890,
+func TestConvertMetricParamRequestToDBModel_Ok(t *testing.T) {
+	testData := []struct {
+		name           string
+		request        *request.LogMetricRequest
+		expectedMetric *models.Metric
+	}{
+		{
+			name: "WithMetricNormalValue",
+			request: &request.LogMetricRequest{
+				Key:       "key",
+				Step:      1,
+				Value:     1.1,
+				RunID:     "run_id",
+				Timestamp: 1234567890,
+			},
+			expectedMetric: &models.Metric{
+				Key:       "key",
+				Value:     1.1,
+				Timestamp: 1234567890,
+				RunID:     "run_id",
+				Step:      1,
+			},
+		},
+		{
+			name: "WithMetricNaNValue",
+			request: &request.LogMetricRequest{
+				Key:       "key",
+				Step:      1,
+				Value:     "NaN",
+				RunID:     "run_id",
+				Timestamp: 1234567890,
+			},
+			expectedMetric: &models.Metric{
+				Key:       "key",
+				Value:     0,
+				IsNan:     true,
+				Timestamp: 1234567890,
+				RunID:     "run_id",
+				Step:      1,
+			},
+		},
+		{
+			name: "WithMetricPositiveInfinityValue",
+			request: &request.LogMetricRequest{
+				Key:       "key",
+				Step:      1,
+				Value:     "Infinity",
+				RunID:     "run_id",
+				Timestamp: 1234567890,
+			},
+			expectedMetric: &models.Metric{
+				Key:       "key",
+				Value:     math.MaxFloat64,
+				Timestamp: 1234567890,
+				RunID:     "run_id",
+				Step:      1,
+			},
+		},
+		{
+			name: "WithMetricNegativeInfinityValue",
+			request: &request.LogMetricRequest{
+				Key:       "key",
+				Step:      1,
+				Value:     "-Infinity",
+				RunID:     "run_id",
+				Timestamp: 1234567890,
+			},
+			expectedMetric: &models.Metric{
+				Key:       "key",
+				Value:     -math.MaxFloat64,
+				Timestamp: 1234567890,
+				RunID:     "run_id",
+				Step:      1,
+			},
+		},
+	}
+
+	for _, tt := range testData {
+		t.Run(tt.name, func(t *testing.T) {
+			metric, err := ConvertLogMetricRequestToDBModel("run_id", tt.request)
+			require.Nil(t, err)
+			assert.Equal(t, tt.expectedMetric, metric)
+		})
+	}
+}
+
+func TestConvertMetricParamRequestToDBModel_Error(t *testing.T) {
+	testData := []struct {
+		name    string
+		request *request.LogMetricRequest
+		error   error
+	}{
+		{
+			name: "WithUnsupportedMetricValue",
+			request: &request.LogMetricRequest{
+				Key:       "key",
+				Step:      1,
+				Value:     struct{}{},
+				RunID:     "run_id",
+				Timestamp: 1234567890,
+			},
+			error: errors.New("invalid metric value '{}'"),
+		},
+		{
+			name: "WithUnsupportedNaNMetricValue",
+			request: &request.LogMetricRequest{
+				Key:       "key",
+				Step:      1,
+				Value:     "UnsupportedNaNValue",
+				RunID:     "run_id",
+				Timestamp: 1234567890,
+			},
+			error: errors.New("invalid metric value 'UnsupportedNaNValue'"),
+		},
+	}
+
+	for _, tt := range testData {
+		t.Run(tt.name, func(t *testing.T) {
+			_, err := ConvertLogMetricRequestToDBModel("run_id", tt.request)
+			assert.Equal(t, tt.error.Error(), err.Error())
+		})
 	}
-	result, err := ConvertMetricParamRequestToDBModel("run_id", &req)
-	require.Nil(t, err)
-	assert.Equal(t, "key", result.Key)
-	assert.Equal(t, int64(1), result.Step)
-	assert.Equal(t, 1.1, result.Value)
-	assert.Equal(t, "run_id", result.RunID)
-	assert.Equal(t, int64(1234567890), result.Timestamp)
 }
diff --git a/tests/integration/golang/aim/metric/search_aligned_test.go b/tests/integration/golang/aim/metric/search_aligned_test.go
index adafbca..8496bb5 100644
--- a/tests/integration/golang/aim/metric/search_aligned_test.go
+++ b/tests/integration/golang/aim/metric/search_aligned_test.go
@@ -7,6 +7,7 @@ import (
 	"context"
 	"database/sql"
 	"fmt"
+	"math"
 	"net/http"
 	"testing"
 
@@ -747,14 +748,20 @@ func (s *SearchAlignedMetricsTestSuite) Test_Ok() {
 				for decodedData[fmt.Sprintf("%v.%d.name", run.ID, metricCount)] != nil {
 					valueKey := fmt.Sprintf("%v.%d.x_axis_values.blob", run.ID, metricCount)
 					xValues[metricCount] = append(xValues[metricCount], decodedData[valueKey].([]float64)[0])
-
 					metricCount++
 				}
 			}
 
 			// Check if the received values for each metric match the expected ones
 			for _, metricValues := range xValues {
-				s.Equal(tt.response, metricValues)
+				for i := 0; i < len(tt.response); i++ {
+					// we can't compare `NaN` values, so this is only one way to do that.
+					if math.IsNaN(metricValues[i]) {
+						s.Equal(math.IsNaN(metricValues[i]), math.IsNaN(tt.response[i]))
+					} else {
+						s.Equal(tt.response[i], metricValues[i])
+					}
+				}
 			}
 		})
 	}
diff --git a/tests/integration/golang/mlflow/run/log_batch_test.go b/tests/integration/golang/mlflow/run/log_batch_test.go
index ca40512..608f544 100644
--- a/tests/integration/golang/mlflow/run/log_batch_test.go
+++ b/tests/integration/golang/mlflow/run/log_batch_test.go
@@ -294,6 +294,57 @@ func (s *LogBatchTestSuite) TestMetrics_Ok() {
 				return metrics
 			}(),
 		},
+		{
+			name: "LogNaNValue",
+			request: &request.LogBatchRequest{
+				RunID: run.ID,
+				Metrics: []request.MetricPartialRequest{
+					{
+						Key:       "key4",
+						Value:     "NaN",
+						Timestamp: 1687325991,
+						Step:      1,
+					},
+				},
+			},
+			latestMetricIteration: map[string]int64{
+				"key4": 1,
+			},
+		},
+		{
+			name: "LogPositiveInfinityValue",
+			request: &request.LogBatchRequest{
+				RunID: run.ID,
+				Metrics: []request.MetricPartialRequest{
+					{
+						Key:       "key5",
+						Value:     "Infinity",
+						Timestamp: 1687325991,
+						Step:      1,
+					},
+				},
+			},
+			latestMetricIteration: map[string]int64{
+				"key5": 1,
+			},
+		},
+		{
+			name: "LogNegativeInfinityValue",
+			request: &request.LogBatchRequest{
+				RunID: run.ID,
+				Metrics: []request.MetricPartialRequest{
+					{
+						Key:       "key6",
+						Value:     "-Infinity",
+						Timestamp: 1687325991,
+						Step:      1,
+					},
+				},
+			},
+			latestMetricIteration: map[string]int64{
+				"key6": 1,
+			},
+		},
 	}
 	for _, tt := range tests {
 		s.Run(tt.name, func() {
diff --git a/tests/integration/golang/mlflow/run/log_metric_test.go b/tests/integration/golang/mlflow/run/log_metric_test.go
index 20d4a12..2d706f7 100644
--- a/tests/integration/golang/mlflow/run/log_metric_test.go
+++ b/tests/integration/golang/mlflow/run/log_metric_test.go
@@ -4,6 +4,7 @@ package run
 
 import (
 	"context"
+	"math"
 	"net/http"
 	"strings"
 	"testing"
@@ -37,39 +38,108 @@ func (s *LogMetricTestSuite) Test_Ok() {
 	})
 	s.Require().Nil(err)
 
-	req := request.LogMetricRequest{
-		RunID:     run.ID,
-		Key:       "key1",
-		Value:     1.1,
-		Timestamp: 1234567890,
-		Step:      1,
+	tests := []struct {
+		name           string
+		request        *request.LogMetricRequest
+		expectedMetric *models.LatestMetric
+	}{
+		{
+			name: "LogMetricWithNormalValue",
+			request: &request.LogMetricRequest{
+				RunID:     run.ID,
+				Key:       "key1",
+				Value:     1.1,
+				Timestamp: 1234567890,
+				Step:      1,
+			},
+			expectedMetric: &models.LatestMetric{
+				Key:       "key1",
+				Value:     1.1,
+				Timestamp: 1234567890,
+				Step:      1,
+				IsNan:     false,
+				RunID:     run.ID,
+				LastIter:  1,
+			},
+		},
+		{
+			name: "LogMetricWithNaNValue",
+			request: &request.LogMetricRequest{
+				RunID:     run.ID,
+				Key:       "key1",
+				Value:     "NaN",
+				Timestamp: 1234567890,
+				Step:      1,
+			},
+			expectedMetric: &models.LatestMetric{
+				Key:       "key1",
+				Value:     0,
+				Timestamp: 1234567890,
+				Step:      1,
+				IsNan:     true,
+				RunID:     run.ID,
+				LastIter:  2,
+			},
+		},
+		{
+			name: "LogMetricPositiveInfinityValue",
+			request: &request.LogMetricRequest{
+				RunID:     run.ID,
+				Key:       "key1",
+				Value:     "Infinity",
+				Timestamp: 1234567890,
+				Step:      1,
+			},
+			expectedMetric: &models.LatestMetric{
+				Key:       "key1",
+				Value:     math.MaxFloat64,
+				Timestamp: 1234567890,
+				Step:      1,
+				RunID:     run.ID,
+				LastIter:  3,
+			},
+		},
+		{
+			name: "LogMetricNegativeInfinityValue",
+			request: &request.LogMetricRequest{
+				RunID:     run.ID,
+				Key:       "key1",
+				Value:     "-Infinity",
+				Timestamp: 1234567890,
+				Step:      1,
+			},
+			expectedMetric: &models.LatestMetric{
+				Key:       "key1",
+				Value:     -math.MaxFloat64,
+				Timestamp: 1234567890,
+				Step:      1,
+				RunID:     run.ID,
+				LastIter:  4,
+			},
+		},
 	}
-	resp := fiber.Map{}
-	s.Require().Nil(
-		s.MlflowClient().WithMethod(
-			http.MethodPost,
-		).WithRequest(
-			req,
-		).WithResponse(
-			&resp,
-		).DoRequest(
-			"%s%s", mlflow.RunsRoutePrefix, mlflow.RunsLogMetricRoute,
-		),
-	)
-	s.Empty(resp)
+	for _, tt := range tests {
+		s.Run(tt.name, func() {
+			resp := fiber.Map{}
+			s.Require().Nil(
+				s.MlflowClient().WithMethod(
+					http.MethodPost,
+				).WithRequest(
+					tt.request,
+				).WithResponse(
+					&resp,
+				).DoRequest(
+					"%s%s", mlflow.RunsRoutePrefix, mlflow.RunsLogMetricRoute,
+				),
+			)
+			s.Empty(resp)
 
-	// makes user that records has been created correctly in database.
-	metric, err := s.MetricFixtures.GetLatestMetricByRunID(context.Background(), run.ID)
-	s.Require().Nil(err)
-	s.Equal(&models.LatestMetric{
-		Key:       "key1",
-		Value:     1.1,
-		Timestamp: 1234567890,
-		Step:      1,
-		IsNan:     false,
-		RunID:     run.ID,
-		LastIter:  1,
-	}, metric)
+			// makes user that records has been created correctly in database.
+			metric, err := s.MetricFixtures.GetLatestMetricByRunID(context.Background(), run.ID)
+			s.Require().Nil(err)
+			s.Equal(tt.expectedMetric, metric)
+		})
+	}
 }
 
 func (s *LogMetricTestSuite) Test_Error() {