Design of separation of pkg/agent/handler test helper functions

[kevindiu]

I have listed up the helper functions in the pkg/agent/handler test implementation.
I have designed the following packageing for the functions.
The reason I design like it is because base on the package name we can easily understand what kind of helper function we are going to use.

Summary:

Refactor the following helper functions in pkg/agent/handler/grpc/handler.go:

// Move these functions to `internal/conv/conv.go` 
func utf8ToSjis(s string) string { // refactor to return error
func utf8ToEucjp(s string) string { // refactor to return error

// Move these functions to `internal/test/data/vector/gen.go `
func genF32Vec(dist vector.Distribution, num int, dim int) [][]float32 { // refactor to return error
func genIntVec(dist vector.Distribution, num int, dim int) [][]float32 {  // refactor to return error
func genSameValueVec(size int, val float32) []float32 {
func convertVectorUint8ToFloat32(vector []uint8) (ret []float32) {
func convertVectorsUint8ToFloat32(vectors [][]uint8) (ret [][]float32) { 

// Move these functions to `internal/test/data/request/insert.go`
func genMultiInsertReq(t objectType, dist vector.Distribution, num int, dim int, cfg *payload.Insert_Config) *payload.Insert_MultiRequest { // refactor to return error

// Move these functions to `internal/test/data/request/Object.go
Inside Test_MultiInsert()
	genSameVecMultiInsertReq := func(num int, vec []float32, cfg *payload.Insert_Config) *payload.Insert_MultiRequest {
	genObjectLocations := func(num int, name string, ip string) *payload.Object_Locations { 

Structure:

internal
|- test
|- |- data
|- |- |- request
|- |- |- |- insert.go
|- |- |- |- |- genMultiInsertReq()
|- |- |- |- |- genSameVecMultiInsertReq()
|- |- |- |- object.go
|- |- |- |- |- genObjectLocations()

|- |- |- vector
|- |- |- |- gen.go
|- |- |- |- |- genF32Vec()
|- |- |- |- |- genIntVec()
|- |- |- |- |- genSameValueVec()
|- |- |- |- |- convertVectorUint8ToFloat32()
|- |- |- |- |- convertVectorsUint8ToFloat32()

|- conv
|- |- conv.go
|- |- |- utf8ToSjis()
|- |- |- utf8ToEucjp()

The following implementation will be removed.

Inside Test_Remove()
	utf8ToSjis := func(s string) string { // duplicated
	utf8ToEucjp := func(s string) string { // duplicated

Inside Test_Insert()
	genF32Vec := func(dist vector.Distribution, dim int) []float32 { // combine with genF32Vec with num prarameter
	genIntVec := func(dist vector.Distribution, dim int) []float32 { // combine with genIntVec with num prarameter

fill() // duplicated with GenSameValueVec()

Also we will implement the BuildIndex() in the test file to return the NGT server object which finished inserting and indexing the random vectors.

func BuildIndex(t ObjectType, num int, dim int, ngtCfg  *config.NGT, srvOpts []service.Option) (Server, error) { 
  // create ngt with ngtCfg
  // create server with srvOpts
  // generate vector
  // insert generated vector into ngt/server
  // create index
  // return server with inserted vec
}

The following helper function will not be refactored.

Inside Test_MultiInsert()
	genAlreadyExistsErr := func(uuid string, req *payload.Insert_MultiRequest, name, ip string) error {

1. genAlreadyExistsErr() will not be refactored as it is only used in Test_MultiInsert()

Original design:

internal/test/data/charset/convert.go

func utf8ToSjis(s string) string {
func utf8ToEucjp(s string) string {

Inside Test_Remove()
	utf8ToSjis := func(s string) string { // duplicated, will remove it
	utf8ToEucjp := func(s string) string { // duplicated above, will remove it

internal/test/data/vector/helper.go (package existed)

func genF32Vec(dist vector.Distribution, num int, dim int) [][]float32 {
func genIntVec(dist vector.Distribution, num int, dim int) [][]float32 {
func genSameValueVec(size int, val float32) []float32 {

Inside Test_Insert()
	genF32Vec := func(dist vector.Distribution, dim int) []float32 { // combine with genF32Vec with num prarameter
	genIntVec := func(dist vector.Distribution, dim int) []float32 { // combine with genIntVec with num prarameter

internal/test/data/request/insert_req.go

func genMultiInsertReq(t objectType, dist vector.Distribution, num int, dim int, cfg *payload.Insert_Config) *payload.Insert_MultiRequest {

Inside Test_MultiInsert()
	genSameVecMultiInsertReq := func(num int, vec []float32, cfg *payload.Insert_Config) *payload.Insert_MultiRequest {
	genObjectLocations := func(num int, name string, ip string) *payload.Object_Locations {
internal/test/data/error/error.go

Inside Test_MultiInsert()
	genAlreadyExistsErr := func(uuid string, req *payload.Insert_MultiRequest, name, ip string) error {

Changes from original design

• return error from the helper functions if necessary
• do not refactor genAlreadyExistsErr() as it is only used in MultiInsert test
• implement genXXXVec() to existing file internal/test/data/vector/gen.go instead of creating new file
• implement utf8ToXXXX() to internal/conv package instead of internal/test/data/ package

[kpango]

@vankichi @kmrmt how do you think?

[vankichi]

well...

• internal/test/data/vector/helper.go (package existed) : how about implement new functions in internal/test/data/vector/gen.go?
• internal/test/data/request/insert_req.go should be internal/test/data/request/insert.go

I do not thinks to implement internal/test/data/error/error.go.
If we decide to implement it, the file name should be internal/test/data/request/error.go or internal/test/data/response/error.go

[vankichi]

so I think it can keep the implementation inside the test function.

You may misunderstand.
I think it does not be needed if it is used only MultiInsert Test

[kevindiu]

I have explained above. We need this to create containErr for MultiInsert test.
e.g. d122eee#diff-059e77835fd022fce77c6202e61ad5a1148830b650e633e31fbee8ab1f824431R6533

[vankichi]

What I thought about is we do not need to implement a helper function if this method will only be used for a specific test.

[kevindiu]

OK, thank you for your explanation. I agree with you that we will NOT refactor the error function genAlreadyExistsErr.

[vankichi]

If the same function will appear in other cases, we should move it to the helper.

[kmrmt]

I have two comments.

• I think helper functions should return error because of proper handling by the function caller.
• Tests without insert must create the NGT index, so it is good for users to get a built index easily.

[kevindiu]

@kmrmt why do we return the server from the BuildIndex function? I suppose that is should only call createIndex() from ngt?

[kmrmt]

@kevindiu It means that BuildIndex returns NGT index which finishes Insert random vectors and CreateIndex

[kevindiu]

@kmrmt which package/file do you think to implement this BuildIndex()?
And I also want to confirm you with the idea you have.

May I confirm that you want a helper function with the following implementation.

func BuildIndex(t ObjectType, num int, dim int, ngtCfg  *config.NGT, srvOpts []service.Option) (Server, error) { 
  // create ngt with ngtCfg
  // create server with srvOpts
  // generate vector
  // insert generated vector into ngt/server
  // create index
  // return server with inserted vec
}

[kmrmt]

@kevindiu First, I think BuildIndex() is implemented in pkg package because Server's implementations are implemented in the package and BuildIndex() will use them.

Second, your suggestion of function interface is very nice:+1:

[kevindiu]

@vankichi what do you think about the BuildIndex() design?

[kevindiu]

I have created an PR for it
#1678

[vankichi]

Before creat PR, you have to gather opinions and describe the plan.

[vankichi]

I recommend you create PR after finishing the discussion.

[kevindiu]

This PR is only for your better image on what will be change.

[kpango]

I think convert.go shoule be located at internal/conv

[kevindiu]

will change it later

[vankichi]

@kevindiu
could you describe in tree diagram which you design in this thread?

e.g.

- internal/test
|- request
|    |- insert.go  // helper for generating insert request

[kevindiu]

I also written it into the PR description for better overall understanding

internal
|- test
|- |- data
|- |- |- request
|- |- |- |- insert.go
|- |- |- |- |- genMultiInsertReq()
|- |- |- |- |- genSameVecMultiInsertReq()
|- |- |- |- object.go
|- |- |- |- |- genObjectLocations()

|- |- |- vector
|- |- |- |- gen.go
|- |- |- |- |- genF32Vec()
|- |- |- |- |- genIntVec()
|- |- |- |- |- genSameValueVec()

|- conv
|- |- conv.go
|- |- |- utf8ToSjis()
|- |- |- utf8ToEucjp()

[kevindiu]

@kmrmt @vankichi
I find that there are one problem of implementing the BuildIndex() design like before.

Since we need to know the inserted vector & ID beforehand to implement the test case
(e.g. in Exisit test, we need to know the generated ID to perform the action, but it is impossible to do that if we design like above)
I think we need to change the design like

func BuildIndex(t ObjectType, insertReq *payload.Multi_insertrequest, ngtCfg  *config.NGT, srvOpts []service.Option) (Server, error) { 
  // create ngt with ngtCfg
  // create server with srvOpts
  // insert vector into ngt/server
  // create index
  // return server with inserted vec
}

to insert the vector through the multiInsertRequest object.

What do you think?

[kevindiu]

FYI, the buildIndex() implementation of the original design is

func buildIndex(ctx context.Context, t request.ObjectType, dist vector.Distribution, num int, insertCfg *payload.Insert_Config, ngtCfg *config.NGT, ngtOpts []service.Option) (Server, error) {
	eg, ctx := errgroup.New(ctx)
	ngt, err := service.New(ngtCfg, append(ngtOpts, service.WithErrGroup(eg), service.WithEnableInMemoryMode(true))...)
	if err != nil {
		return nil, err
	}

	s, err := New(WithErrGroup(eg), WithNGT(ngt))
	if err != nil {
		return nil, err
	}

	if num > 0 {
		reqs, err := request.GenMultiInsertReq(t, dist, num, ngtCfg.Dimension, insertCfg)
		if err != nil {
			return nil, err
		}
		if _, err := s.MultiInsert(ctx, reqs); err != nil {
			return nil, err
		}
		if _, err := s.CreateIndex(ctx, &payload.Control_CreateIndexRequest{PoolSize: 100}); err != nil {
			return nil, err
		}
	}

	return s, nil
}

[kmrmt]

My just idea is using arguments like overwriteIDs []string and overwriteVectors [][]float32.

func BuildIndex(..., overwriteIDs []string, overwriteVectors [][]float32) {
	...
	var reqs *payload.Insert_MultiRequest
	// overwrite IDs if needed
	if overwriteIDs != nil {
		for i, id := range overwriteIDs {
			reqs.Requests[i].Vector.Id = id
		}
	}

	// overwrite Vectors if needed
	if overwriteVectors != nil {
		for i, v := range overwriteVectors {
			reqs.Requests[i].Vector.Vector = v
		}
	}
}

[kevindiu]

@vankichi what do you think?

[kevindiu]

@kmrmt @vankichi
Sorry I think that I missed 3 helper functions here.

For convertXXX(), I think we can put it in internal/test/data/vector/vector.go or gen.go, what do you think?
For fill(), the implementation is the same as GenSameValueVec() so I will remove it.

func convertVectorUint8ToFloat32(vector []uint8) (ret []float32) { // put it in internal/test/data/vector package
	ret = make([]float32, len(vector))
	for i, e := range vector {
		ret[i] = float32(e)
	}
	return
}

func convertVectorsUint8ToFloat32(vectors [][]uint8) (ret [][]float32) { // put it in internal/test/data/vector package
	ret = make([][]float32, 0, len(vectors))
	for _, v := range vectors {
		ret = append(ret, convertVectorUint8ToFloat32(v))
	}
	return
}

func fill(f float32, dim int) (v []float32) { // duplicated impl, will remove it
	v = make([]float32, dim)
	for i := range v {
		v[i] = f
	}
	return
}

[vankichi]

For convertXXX(), I think we can put it in internal/test/data/vector/vector.go or gen.go, what do you think?

could you please let us know the reason?

For fill(), the implementation is the same as GenSameValueVec() so I will remove it.

OK

[kevindiu]

could you please let us know the reason?

I think it is reuseable to implement test case for uint object type, so I think we should put it into internal test package.
Moreover actually the conversion logic is implemented in the genIntVec(), I think I can refactor the genIntVec() to use these functions.

[kmrmt]

both of them LGTM

[vankichi]

where are they from?

[kevindiu]

all these functions are from Search test func.

See changes:
ConvertXXX: ff590e7
fill(): d131d1f

[vankichi]

#1678