Python client changes for json inputs - #232

maybe this stuff really isn't necessary :P

wdym?

there's a lot of logic to try to figure out a reasonable request_id if the user didn't pass one in, e.g. use the url, or the args, which seems "a bit extra"

Oh ok, as per my other comment (18 hours ago), can we just generate a UUID?

since we have a function to predict on multiple inputs, I added this so people could pass in a mix of urls/jsons as inputs. Not sure if that'll happen

Might be good to add an EndpointResponse class for symmetry?

Think you typically do = None for Optional stuff.

I would group these conditions together:

If `return_pickled` is True, the key will be "result_url" and the value will be a signedUrl that <blah>
If `return_pickled` is False, the key will be "result" and the value is the arbitrary JSON returned by the endpoint's `predict` function.

Might be good to add an EndpointResponse class for symmetry?

Per above, -> EndpointResponse might be a handy result and annotation.

wdym?

When/why would a user specify this? I was imagining this would be populated by us server-side.

The thing this request_id is trying to solve is that a user needs to be able to associate model inference results with a given model input. For example, for the nucleus integration we need to maintain some map from s3URL of input images to task results, so this request_id can be set to that s3URL. Before, I was implicitly using the url as the request_id, but we don't necessarily have urls anymore.

I suppose that we could just keep the association implicitly in the order of some list somewhere but then the user would have to do that too.

Ok it sounds like you're basically using this as a reference_id then? Can we just have our client generate a UUID? Then the nucleus integration (which one day will be server-side) can just maintain that Request -> Response mapping, with the hash key being this UUID.

I left in the ability for clients to specify their own request_ids, but otherwise the client will generate a uuid for the request_id

I would err on the more restrictive side and not allow request_ids, because I'm imagining a hapless user passing in "42" hardcoded and never changing it.

(probably should have specified this but) The uniqueness condition is actually more permissive than "globally unique", but instead is "the EndpointRequests producing a given BatchResponse must have distinct request_ids". I think that mostly gets rid of the hardcoding problem? (i.e. it doesn't matter if the user runs the script twice with the same value of request_id, but if the user hardcodes a value for request_id somewhere and copies the code that would be a problem)

I do think we should allow users to set request_ids though, since otherwise it's much more annoying for users to keep track of an association from EndpointRequest/EndpointResponse to something a user cares about, e.g. if the user wants to run inference on N images, they need to keep track of which EndpointResponse gets associated with which image.

With user-specified request_ids the user can do endpoint_response_for_image_i = job.responses[request_id_for_image_i]; with non-user-specified request_ids the user would need to do

# Go through the list of EndpointRequests to find the request_id corresponding to a given image, comparing either `url` or `args`. Comparing values seems reasonable in the `url` case but ugly in the `args` case.
# job.responses[the request_id we found]

or use the ordering of EndpointRequest fed in and the corresponding ordering of EndpointResponses (currently EndpointResponses are returned in a dictionary though)
(or something I haven't thought of)

One simple thing we could do to catch errors earlier is just to check that the user passes a set of distinct request_ids to the batch_predict function at the very least

Oh I see. I think this is happening because the client library is still operating under the paradigm where batch jobs are tied to endpoints instead of being standalone. In the eventual state, I would expect the batch request to be its own class (not a predict_batch method off of an endpoint class), and result would be a url to a file/directory that contains outputs, with the relative order not being guaranteed anyway. In other words, this is needed only because have to support the current janky client-side integration. In that case I guess this is fine, but prior to launch I definitely would want request_id to be gone altogether from the client, so please add a TODO.

I was tempted to suggest that we call this reference_id, but that's misleading in a different way (Nucleus actually persists those, so for us to use them as ephemeral client-side IDs could be confusing due to inconsistency).

Yeah it's pretty much there because of the async batch class

Wonder if we should have these two things be a Union (we'd still have to do the XOR check because Union is really just a hint, from a type annotation perspective).

I sorta am leaning away from putting the two into a Union. We'd have to keep track of whether the data represents a url or args (which we technically could do by looking at their types, but this feels wrong), and keeping them separate seems like the easiest way to do this. The stuff stored at url/args gets put into separate fields in the eventual http request's json anyway, so I think it's simpler to keep things separate.

Yup that makes sense.

Related to the Union comment, think this is happening enough where we'd want a helper function.

Not really part of this PR but maybe we can take a pass at removing all "hmi" stuff from user-facing names?