Exception class for failed task

[djmattyg007]

Right now, there is no way to easily raise an exception for a failed task. When I use the built-in "wait for task" functionality, I'd like a way to get it to automatically raise an exception if the task I've waited for did not succeed. This could happen automatically as part of calling the method, or it could take the form of a raise_for_status() method (like what the requests library exposes on response objects).

The latter suggestion is complicated by the fact that right now, not all methods that return task data return the actual Task object. Some of them return dictionaries while others return instances of the Task class. I think this inconsistency would need to be addressed at the same time.

It is worth noting that the MeilisearchApiError exception contains very similar fields to what is available in the error data in the API task data. Unfortunately it also contains other information that make it difficult to easily re-use for the purpose I've described (a "failed task" exception).

[sanders41]

raise_for_status won’t work here because the status returned to wait_for_task for a failed task is still going to be 200. You could take the information returned by wait_for_task and raise and your own exception from that information. The purpose of wait_for_task is just to check if Meilisearch is still processing the task, so with this in mind it shouldn’t raise an exception itself unless it times out.

[sanders41]

The latter suggestion is complicated by the fact that right now, not all methods that return task data return the actual Task object. Some of them return dictionaries while others return instances of the Task class. I think this inconsistency would need to be addressed at the same time.

Can you point out where dictionaries are returned? I thought all of these were updated, but it sounds like you found some that were missed? Or are are you using the TaskHandler class directly?

[alallema]

There is nothing to add; @sanders41 sander has already answered perfectly! wait_for_task is not intended to raise an exception but only to inform that a task is well finished.
I would also be curious to know which functions still return dictionaries. It is possible that there have been omissions that should be fixed.

[sanders41]

@alallema I have found at least some of them. The tasks from TaskHandler, and the task methods in Client still return dictionaries. I can do a PR to update this, but it's a breaking change so I wanted to make sure you want to do that first.

[alallema]

@sanders41, that would be great! Yes, I think it's better to have everything coherently, even if it brings a breaking change.
And Meilisearch v.1.2.0 is scheduled for June 5th, so maybe we should take the opportunity to release it at the same time

[djmattyg007]

raise_for_status won’t work here because the status returned to wait_for_task for a failed task is still going to be 200. You could take the information returned by wait_for_task and raise and your own exception from that information. The purpose of wait_for_task is just to check if Meilisearch is still processing the task, so with this in mind it shouldn’t raise an exception itself unless it times out.

To be clear, I wasn't asking specifically for a raise_for_status method. I was hoping for something like the raise_for_status method that requests provides. I absolutely appreciate that a simplistic "raise for status" method doesn't make sense in this context.

However I do believe something like this is a great idea. It greatly reduces the amount of boilerplate code users of this library need to write to be able to write correct code with good error handling.

[sanders41]

A way do to this would be to add raise_for_status as a parameter to the method. Then if the status is failed raise an exception. So it would look something like this:

def wait_for_task(
        self,
        uid: int,
        timeout_in_ms: int = 5000,
        interval_in_ms: int = 50,
        raise_for_status: bool = False,
    ) -> Task:
        start_time = datetime.now()
        elapsed_time = 0.0
        while elapsed_time < timeout_in_ms:
            task = self.get_task(uid)
            if task.status not in ("enqueued", "processing"):
                if raise_for_status and task.status == "failed":
                    raise MeilisearchFailedTaskError(f"Task {uid} failed")
                return task
            sleep(interval_in_ms / 1000)
            time_delta = datetime.now() - start_time
            elapsed_time = time_delta.seconds * 1000 + time_delta.microseconds / 1000
        raise MeilisearchTimeoutError(
            f"timeout of ${timeout_in_ms}ms has exceeded on process ${uid} when waiting for task to be resolve."
        )

Then, for example, index.wait_for_task("some_uid", raise_for_status=True) would raise an exception on a failed task.

@alallema thoughts? This would be the only SDK to implement this unless the team wanted to add it everywhere.

[alallema]

I find the idea of how to solve this problem very nice and interesting however I do not think that this is very hard for the user to raise his own exception and especially I think that if we implement this new feature it should be present on all SDKs. We are already in a great debate about this wait_for_task method as you know, so I allow myself to add this to the already present discussion.