Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return pointer instead of value in SeldonApiClient methods #2014

Closed
RafalSkolasinski opened this issue Jun 26, 2020 · 5 comments
Closed

Return pointer instead of value in SeldonApiClient methods #2014

RafalSkolasinski opened this issue Jun 26, 2020 · 5 comments
Assignees

Comments

@RafalSkolasinski
Copy link
Contributor

RafalSkolasinski commented Jun 26, 2020

Methods defined by SeldonApiClient interface in executor return payload.SeldonPayloads via value not reference.

Is this a potential bottleneck? Could we gain by returning pointers there?

Conscious this is for consistency across all functions above, but why are we 
returning `payload.ModelMetadata` by value instead of reference/pointer? 
It seems like it would be quite inefficient to copy all the values that are 
returned by the client

_Originally posted by @axsaucedo in #2005 (comment)

@glindsell glindsell changed the title Return reference instead of value in SeldonApiClient methods Return pointer instead of value in SeldonApiClient methods Jun 26, 2020
@glindsell
Copy link
Contributor

glindsell commented Jun 26, 2020

payload.SeldonPayload is actually an interface which can hold either a struct or a pointer to a struct.
payload.ModelMetadata however, is a struct so a pointer should be returned.

@RafalSkolasinski
Copy link
Contributor Author

This is perfect @glindsell, thanks for the insight! I had to forgot about that Go nuance :)

@RafalSkolasinski
Copy link
Contributor Author

Going down on Golang good practices I now wonder if it is best to have

  1. SeldonApiClient.Metadata method that returns payload.SeldonPayload storing raw payload and SeldonApiClient.ModelMetadata that returns model metadata decoded to payload.ModelMetadata struct

or

  1. Have one method in SeldonApiClient that returns payload.SeldonPayload and extend payload.SeldonPayload interface to have a method that decodes payload to ModelMetadata struct.

@ukclivecox
Copy link
Contributor

I think the SeldonPayload is meant to be very generic and not really get into the machine learning specifics. So I'd suggest not unless a good reason.

@RafalSkolasinski
Copy link
Contributor Author

I think the SeldonPayload is meant to be very generic and not really get into the machine learning specifics. So I'd suggest not unless a good reason.

Ok, that basically cross away option 2 meaning that what is currently in metadata PR is right up to the return value instead of pointer for optimisation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants