Add a wait method for batches

[sarjevane]

A wait method for batches

That would be in addition of being available as an argument in the add_job, retry and close methods.

Why a new method?

The "refresh-and-check-state" mechanism is currently re-implemented in several methods, but not accessible to the user, only as an argument in other methods.

The reasons to add this method would be:

• Maintainability: the code would not be duplicated anymore, and the implementation of further features (such as Add retries when one request returns a 500 while waiting for results (workloads/batches/jobs) #110 ) would have to be implement in one spot only.
• Flexibility: If for any reason you "forget" to - or simply cannot - add the wait, or if the waiting loop breaks down (e.g. until the above-mentioned issue is not implemented), then you have no easy way of doing it aside from rewriting the wait loop yourself, potentially introducing some errors yourself (e.g. forgetting some states).
• Performance: It would allow to do some processing between the moment jobs are added (and not waited) and the moment when the user actually needs the results (without adding new jobs or closing the batch to force the wait mechanism)

Naming

The name of the method itself is up for discussion.

I see the following options, but there obviously could be more:

• my_batch.wait()
• my_batch.await()
• my_batch.wait_for()
• my_batch.wait_settled() (inspired by JS promises)

Arguments

The methods could have the following argument:

• retry5xx: boolean to retry (not raise an error) on 5xx status codes from the backend (default: true) - implemented with Add retries when one request returns a 500 while waiting for results (workloads/batches/jobs) #110

[oliver-gordon]

I'm not entirely convinced the use of wait as an arg or method is necessary feature, honestly.

When I execute a create style method in an SDK, I don't anticipate this will guarantee me the return of a final response, I mostly anticipate I'll get a 200 or something to indicate creation.

All this is really doing is calling another method get_batch which the users have complete access to and could more conveniently execute in a loop themselves.

I'll agree however, that if this approach exists I think a method is preferable to an Arg. I like the more functional approach.

But this type of feature anticipates that things execute incredibly rapidly. Having a script execute for 5 hours simply making pings is not a useful design in the SDK.

[MatthieuMoreau0]

Having a script execute for 5 hours simply making pings is not a useful design in the SDK.

Any alternative suggestions to handle variational algorithms (where you need to post-process the results of a job to compute the parameters for the next one) ? We could do websockets in place of pings I guess?

[oliver-gordon]

Well, the only thing wait is triggering is that we call get_batch, another public method to populate the class eventually. We could just thrust the job of calling get_batch onto the user. Since this change would not actually make a difference in the behavior, I'm not strongly advocating we move to this. Some form of wait method would be fine I reckon.

create_batch semantically only makes sense trusting that everything works on the first execution.

There are a bunch of reasons I wouldn't advocate for web sockets for something like this that would be better discussed offline.

[sarjevane]

Well, the only thing wait is triggering is that we call get_batch, another public method to populate the class eventually. We could just thrust the job of calling get_batch onto the user

That is currently true, it only hides a loop and a fetch for now. The benefits are limited (but still there IMO) for now. The value will increase as soon as we implement #110 though, or any similar changes. With that in mind, would you be a bit more inclined to move in that direction @oliver-gordon ?

[oliver-gordon]

Maybe the tickets missing details but 500s are rarely worth a retry because they occur when somethings actually broken. 502s perhaps because these raise when our service-to-service communications timeout, but 500s typically raise due to a runtime error.

I suppose if I understand that the wait method is to supplement the whole 'get_batch` process? Because this SDK is designed with some hard inconsistencies so it's hard to gauge if we're just introducing another way to do something or a direction we want to steer the entire interface towards?

[MatthieuMoreau0]

Thanks a lot for bringing this up!

Letting users wait for the results of an existing batch seems like a basic feature that should for sure be available in the SDK. I'm pretty sure we had this at some point but it looks like it was removed by mistake when the fetch_results argument of the get_batch function was deprecated.

We should definitely work on that. As per the interface and implementation for this feature, I also think adding a dedicated method would be nice for the reasons you mentioned. I think the waitargument is a bit wonky as it completely changes the behaviour of the function it applies to. I think letting users make an explicit call to the dedicated method is simple and slightly clearer. It would look like

batch = create_batch(sequence)
batch.await()

postprocess(batch.jobs[0].results)

No strong opinion on the naming just yet, I like await

[oliver-gordon]

await wouldn't be appropriate since it's an actual Python async reserved word. Whether or not the namespace would be affected it's something we proactively avoid.

[MatthieuMoreau0]

Yeah good point, I don't like it no more

[sarjevane]

Rust uses the .await syntax, that was my source of inspiration.

I think the waitargument is a bit wonky as it completely changes the behaviour of the function it applies to

I definitely agree with this. However we will have to keep it (but deprecate) to avoid breaking backward compatibility.

it was removed by mistake when the fetch_results argument of the get_batch function was deprecated.

I was unaware of this function - I only looked at the Batch class. The deprecation was introduced but the behavior was dropped as well though, which I would prefer not to happen in this instance. We should (IMO) deprecate the field but keep the behavior, at least until we move to 1.x.

[MatthieuMoreau0]

Yes we try to enforce backwards compatibility unless changing the major, this was definitely a mistake; the functionality should have been maintained. And since we wanted to keep the "wait" feature we should have introduced a non-deprecated way to do it as well imo