Trying to fix issue with NaN, Inf, -Inf

[dsuhinin]

Current solution

to be fully compatible with original MLflow we have to use the same marshaller/unmarshaller -> protojson from google.golang.org/protobuf/encoding/protojson package. By using such marshaller/unmarshaller helps out of the box correctly handle unusual cases related to NaN, Inf and -Inf values.

Previous description (for context)

A bit of context and answers:

• Why bring in the custom json source code?
  -- custom json serialiser allows to correctly wrap/unwrap such a constants like: Nan, Inf, +Inf, -Inf. Unfortunately original custom json serialiser worked a bit differently then MLFlow behaves. It allows parsing Nan, Inf, +Inf, -Inf when they are literal values like:

{
  "value_1": NaN,
  "value_2": Inf,
  "value_3": -Inf
}

but unfortunately Mlflow sends such values as a string values like:

{
  "value_1": "NaN",
  "value_2": "Inf",
  "value_3": "-Inf'
}

that's why I've changed it to support what Mlflow wants and as a result pull it to our codebase.

• Why did response serialization need a change?
  -- probably the same answer like first item. All the Nan, Inf, +Inf, -Inf values should be serialised as a string values.
• Is any python unit test solved with any of this?
  -- yes, now all the rest tests are green.

[nojaf]

@dsuhinin could you add a high level walkthrough of this in the description please?
By the code alone, I cannot tell why you are doing what you are doing.

Why bring in the custom json source code?
Why did response serialization need a change? Is any python unit test solved with any of this?

[dsuhinin]

@dsuhinin could you add a high level walkthrough of this in the description please? By the code alone, I cannot tell why you are doing what you are doing.

Why bring in the custom json source code? Why did response serialization need a change? Is any python unit test solved with any of this?

Is any python unit test solved with any of this?

• probably this one you can check in README.md file. Yes, the last rest endpoint test has been fixed by this.

[dsuhinin]

@dsuhinin could you add a high level walkthrough of this in the description please? By the code alone, I cannot tell why you are doing what you are doing.

Why bring in the custom json source code? Why did response serialization need a change? Is any python unit test solved with any of this?

answered in a description.

[nojaf]

Thanks, Dmitry! I really appreciate the extra context.

You probably checked this, but was it not possible to hook into the unmarshalling of a single property? It feels a bit cumbersome to include all the JSON source code. We should probably document this in the README. Any new contributor with Go knowledge will most likely be put off by this JSON code. It is also unclear which parts we changed and whether it's possible to update the JSON code if needed.

If there's truly no better way, then I think this is fine, but it would be best to summarize this in an ADR note.

[dsuhinin]

Thanks, Dmitry! I really appreciate the extra context.

You probably checked this, but was it not possible to hook into the unmarshalling of a single property? It feels a bit cumbersome to include all the JSON source code. We should probably document this in the README. Any new contributor with Go knowledge will most likely be put off by this JSON code. It is also unclear which parts we changed and whether it's possible to update the JSON code if needed.

If there's truly no better way, then I think this is fine, but it would be best to summarize this in an ADR note.

emmm, Im not sure, about another approach really. Maybe, maybe it could be but it is hard to say. Logical way to solve this custom behaviour of MLFlow just to have own marshaler/unmarshaller. This guy also created own package for that -> https://github.com/xhhuango/json so it means that this is not really possible to simply inject something into a parser.

If there's truly no better way, then I think this is fine, but it would be best to summarize this in an ADR note.

• I think that this is most resonable way if we want to handle custom surprises from MLFlow.

[dsuhinin]

I can't see any problem to use own json marshaller/unmarshaller, because json itself is not something that changes pretty often. unfortunately we can't extend std lib.

[nojaf]

Can't we do something along the lines of

package main

import (
	"encoding/json"
	"fmt"
	"strconv"
)

// Custom struct with a string field that should be converted to float during JSON marshaling/unmarshaling
type CustomStruct struct {
	Name  string  `json:"name"`
	Value FloatAsString `json:"value"`
}

// Define a custom type that implements json.Marshaler and json.Unmarshaler
type FloatAsString float64

// Custom JSON marshal function
func (f FloatAsString) MarshalJSON() ([]byte, error) {
	// Convert the float value to a string and return it as a JSON string
	return json.Marshal(fmt.Sprintf("%.2f", f))
}

// Custom JSON unmarshal function
func (f *FloatAsString) UnmarshalJSON(data []byte) error {
	// Unmarshal the data into a string, then parse the string as a float
	var s string
	if err := json.Unmarshal(data, &s); err != nil {
		return err
	}

	// Convert the string to a float
	parsed, err := strconv.ParseFloat(s, 64)
	if err != nil {
		return err
	}

	*f = FloatAsString(parsed)
	return nil
}

func main() {
	// Example JSON with a string value for "value"
	data := []byte(`{"name": "example", "value": "123.45"}`)

	var cs CustomStruct
	err := json.Unmarshal(data, &cs)
	if err != nil {
		fmt.Println("Error unmarshaling:", err)
		return
	}

	fmt.Printf("Unmarshaled struct: %+v\n", cs)

	// Marshal it back to JSON
	jsonData, err := json.Marshal(cs)
	if err != nil {
		fmt.Println("Error marshaling:", err)
		return
	}

	fmt.Println("Marshaled JSON:", string(jsonData))
}

[jgiannuzzi]

what I had in mind last time we discussed this is exactly what Florian proposed above in #45 (comment)

[dsuhinin]

@nojaf @jgiannuzzi protojson doesn't provide any information about the filed, that's why I've decided to fallback to json validation. a bit ugly and strange but there is no new failing tests. what do you think?

[nojaf]

This seems reasonable, we want to know what is up when things fail.

[dsuhinin]

correct. I tried to only keep protojson but it is absolutely trick then to find the field name where it failed. We can document that probably, because it is a bit confusing.

[jgiannuzzi]

why does this need to change in this PR?

[dsuhinin]

by some reason it started failing. Ill double check that.

[nojaf]

I've seen this before, we should update Go and/or the linter. I believe this was fixed somewhere.

[jgiannuzzi]

could you please update the PR description with the current approach? and maybe keep the previous description under a heading "Previous description (for context)"?

[dsuhinin]

could you please update the PR description with the current approach? and maybe keep the previous description under a heading "Previous description (for context)"?

sure, Ill update description.

[nojaf]

Looks great to me!
I'm glad this turned out so elegantly!
Nice job, both of you!