Redesign attribute.go

Declare Attributes

Currently, users write the following code to declare attributes:

var distributedTrainingAttributes = attribute.Dictionary{
	"train.num_ps":        {attribute.Int, 0, "", nil},
	"train.num_workers":   {attribute.Int, 1, "", nil},
	"train.worker_cpu":    {attribute.Int, 400, "", nil},
}

This forces us to expose types like attribute.Description which should be internal. Also, this exposes constants like attribute.Int, which should be hidden as well.

Following the design of https://golang.org/pkg/flag/, the declaration should be

var distributedTrainingAttributes = attribute.NewDictionary{).
	Int("train.num_ps", 0, "", nil),
	Int("train.num_workers", 1, "", nil),
	Int("train.worker_cpu", 400, "", nil),
}

We must be very careful about the abuse of interface {}. In the current API, users can passing value as an interface {} type, which might not match the specified type.

Also, users can pass in any reflect.Type as the type, not necessarily chosen from the pre-defined list.

Changing to Int("train.num_pos", 0, ...) makes the compiler checks the type and value are matched.

Abuse of Global Variables

The following global variables are not needed,

https://github.com/sql-machine-learning/sqlflow/blob/2988c7b155a813c2a5a70035209fb5d523b48df5/pkg/attribute/attribute.go#L165-L172

as they are simply another form of

https://github.com/sql-machine-learning/sqlflow/blob/2988c7b155a813c2a5a70035209fb5d523b48df5/pkg/attribute/attribute.go#L201-L212

Types and Constants Should've Hidden

No need to define type unknown and constant Unknown because reflect.Type is an interface, which could be assigned nil to represent unknown types.

https://github.com/sql-machine-learning/sqlflow/blob/2988c7b155a813c2a5a70035209fb5d523b48df5/pkg/attribute/attribute.go#L46

To support the optional default value

package main

import (
	"log"
	"reflect"
)

// Dictionary contains the description of all attributes
type Set struct {
	d      map[string]*description
	recent string // Recently added name.
}

type description struct {
	typ  reflect.Type
	val  interface{}
	doc  string
	chkr func(i interface{}) error
}

func NewSet() *Set {
	return &Set{
		d:      make(map[string]*description),
		recent: "",
	}
}

// Int declares an attribute of int-typed in Dictionary d.
func (s *Set) Int(name string, doc string) *Set {
	s.d[name] = &description{
		typ: reflect.Type(int),
		doc: doc,
	}
	s.recent = name
	return s
}

func (s *Set) Default(v interface{}) *Set {
	if t := reflect.TypeOf(v); t != s.d[s.recent].typ {
		log.Fatalf("The type of %s is declared %v, but the default value %v has a different type %v", s.recent, s.d[s.recent].typ, v, t)
	}
	return s
}

func main() {
	NewSet().Int("num_class", "Number of classes").Default(5) // Runs well.
	NewSet().Int("num_class", "Number of classes").Default(1.5) // log.Fatal
}

How about use optional package like https://pkg.go.dev/github.com/markphelps/optional?tab=doc ?

The optional package is implemented like:

// Int is an optional int.
type Int struct {
    value *int
}

// NewInt creates an optional.Int from a int.
func NewInt(v int) Int {
    return Int{&v}
}

func main() {
    b1 := Int() // nil int
    b2 := NewInt(0) // non nil int with value 0
}

The attribute code may like:

func (d Dictionary) Int(name string, defaultValue optional.Int, 
                        doc string, checker func(int) error) {
  ...
}

var dict = Dictionary{}.
   Int("num_class", optional.Int{}, "doc1", checker1). // default value is nil
   Int("attr_with_default_value", optional.NewInt(0), "doc2", checker2) // default value is 0

I am afraid that optional is not a good choice. There is a good reason https://stackoverflow.com/a/2032160/724872 that Go doesn't support optional parameters. However, the optional package provides a way to walk around this reasonable constraint.

I would prefer the .Default solution in #42 (comment) for the following reasons:

It removes the constants Bool, String,Int, IntList -- Occam's Razor.
It hides the type Description -- Occam's Razor to minimize the API.
It doesn't brutely add optional parameters thus follows the Go convention.
It is more readable than the current way of constructing Description struct manually and than the use of optional package.

@wangkuiyi

@wangkuiyi If we use #42 (comment), how about just write the Dictionary.Int code like:

Dictionary.Int(name string, value interface{}, doc string, checker func(int) error)

instead of 2(or 3) methods:

Dictionary.Int(name string, doc string)
Dictionary.Default(value interface{})
// Maybe there would be another method to set checker,
// or set checker in Dictionary.Int method

In this way, we can:

no need Default method.
no need recent field in Set(or Dictionary).
checking whether the default value is valid can be done inside Dictionary.Int.

@sneaxiy

@sneaxiy I am afraid that we cannot. If we do so, users need to write

var a = 1.0f
attribute.NewDictionary().Int("name", &a, ...)

Go syntax doesn't allow him/her to write&1.0f directly. This would make the code pretty messy.

@wangkuiyi

@wangkuiyi Go can convert primitive types like int, float to interface{}. The following codes run well in Go.

package main

import "fmt"

func Print(value interface{}) {
   fmt.Println(value)
}

func main() {
   Print(3)
   Print(float32(-1.2))
}

@wangkuiyi

@wangkuiyi Go can convert primitive types like int, float to interface{}. The following codes run well in Go.

Fantastic! I agree with your proposal. Thank you for letting us know this property of Go!

@wangkuiyi

@wangkuiyi The attribute package has been refined by the following PR:

Add named type methods like Dictionary.Int: Add typed API to pkg/attribute sqlflow#2486 , https://github.com/sql-machine-learning/sqlflow/pull/2496/files
Replace attribute definition with the new added named type methods: Update attribute definition by named type methods and hide attribute.Description sqlflow#2504
Remove global variables xxxJSON: Remove XGBoostObjectiveJSON sqlflow#2508 , Remove ModelParameterJSON and OptimizerParameterJSON in attribute package sqlflow#2520
Hide some unnecessary exported structs and global variables like attribute.Int, attribute.Description: Update attribute definition by named type methods and hide attribute.Description sqlflow#2504

Maybe we can close this issue now.

Navigation Menu

Repository navigation

Redesign attribute.go #42

Declare Attributes

Abuse of Global Variables

Types and Constants Should've Hidden

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Redesign attribute.go #42

Description

Declare Attributes

Activity

wangkuiyi commented on Jun 17, 2020

Abuse of Global Variables

wangkuiyi commented on Jun 17, 2020

Types and Constants Should've Hidden

wangkuiyi commented on Jun 18, 2020

sneaxiy commented on Jun 18, 2020

wangkuiyi commented on Jun 18, 2020

sneaxiy commented on Jun 19, 2020

wangkuiyi commented on Jun 19, 2020

sneaxiy commented on Jun 19, 2020

wangkuiyi commented on Jun 19, 2020

sneaxiy commented on Jun 24, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions