Closed caiquejjx closed 1 week ago
Hey @caiquejjx 👋
Typically, we ask for a Colab to showcase the changes, which significantly speeds up the review process.
I made one for us this time: https://colab.research.google.com/drive/1xNixTC2xljopKhSpatDhYr2pCY0b5HvG#scrollTo=xI1cabXdatjr
I like the change, but wonder what the broader impact would be. Whether there's anyone relying on us failing silently.
Thanks @LinasKo! yeah it makes sense
Hi @caiquejjx, I'll be looking at this, and if you're around - let's get it merged!
I've tested the solution and it works well, but I'd like a slightly safer approach to parsing the error.
Here's what I have in mind:
def _parse_upload_error(self, error: rfapi.UploadError) -> str:
dict_part = str(error).split(": ", 2)[2]
# Could be done with ast.literal_eval, but this is safer.
if re.search(r"'\w+':", dict_part):
temp_str = dict_part.replace(r"\'", "<PLACEHOLDER>")
temp_str = temp_str.replace("'", '"')
dict_part = temp_str.replace("<PLACEHOLDER>", "'")
parsed_dict: dict = json.loads(dict_part)
message = parsed_dict.get("message")
return message or str(parsed_dict)
Could you replace the method (and import re
instead of ast
), and then test it?
Let's also add a correct annotation test, with annotation format such as this:
Hey @LinasKo sure thing, I'll work on these soon and I guess we should also have automated tests right? (not sure if it was what you meant)
Aye, I wasn't too clear. A Colab will be fine this time - no need for unit tests.
@tonylampada, could you please have a look at this? I believe it is ready to merge.
@LinasKo I think uploading with the CLI relies on the previous more lenient handling of annotation errors. Can you test that and see if it still has the same behaviour when importing datasets with the CLI?
Also, I think the backend already provides valid json in case of error, it just seems weird parsing it using regex like that.
@tonylampada, here's the repr
of the error. It's not valid json - the strings are declared with '
!.
UploadError("save annotation for MzqOjxfl22A5rxmFhrUI / bad response: 400: {'message': 'Image was already annotated.', 'type': 'InvalidImageException', 'hint': 'This image was already annotated; to overwrite the annotation, pass overwrite=true as a query parameter.'}")
I very much agree it's weird! The correct solution would be to change wherever the json is produced. Alternatively, let's use the whole string. Also, we can avoid all of this with ast.literal_eval
, as per original suggestion by @caiquejjx, but that's a slight risk if the API error ever contains user-defined data.
CLI test results coming soon.
@tonylampada, I've tested:
However, I also labelled a keypoints dataset, created a version and attempted to download it, which failed. I think it's related to json, but unrelated to my changes.
Could you test if the following works?
roboflow download https://app.roboflow.com/linas-ws/keypooint-test/1 -l dataset-kp
@tonylampada, responding to your Loom, I see the bigger picture now. We're both working towards the same goal of not hiding useful error messages.
The problem is that dataset import captures the result of project.single_upload
and logs it with _log_img_upload
, whereas uploading a single image/annotation completely ignores the result.
The correct solution would keep one of these approaches. Should we implement an equivalent of _log_img_upload
in roboflow/roboflowpy.py::upload_image()
, and then remove the raise
introduced by this PR?
Aforementioned loom here just for reference (so we don't exclude @caiquejjx from the conversation :-)) https://www.loom.com/share/5f7a6520e0364b7588fd6130c682e0ef
@LinasKo here's what I'm thinking.
project.single_upload
has an "identity crisis" because it's not exactly "single" upload, is it? :-)
It can upload:
Then there's the question of what do we do when something goes wrong with one of those operations. Ideally we should decouple those in different methods, but that would be a breaking change, probably not worth it. So, scratch that.
Then, when should single_upload throw an error? One possible answer is:
I think this kinda makes sense from the perspective of what error means. But I don't like it. Because it's a little more complex to implement inside (single_upload) and that complexity also leaks by forcing the caller to know when to handle and/or look inside the results.
So I'm tending to think that a better answer is:
Now, duplicating _log_img_upload
is something I don't like. DRY, right?
Maybe we should move that responsibility into single_upload
as well, before returning the result dic. wdyt?
@tonylampada thanks for the loom! one note is that single_upload
kinda did that before already but the problem was that the upload
method didn't return anything and one of the reasons I went for raising is because people probably don't check the return of the upload
since it doesn't returns anything, so besides the change on single_upload
we should also start returning the result in upload
, right? or maybe logging
we should also start returning the result in upload, right?
Yup. That makes sense
or maybe logging
I think it might be better to move the logging responsibility into single_upload.
I'm writing some tests for my PR (that maintains the compatibility with CLI), and this PR breaks the upload parse in some cases, I'll describe it better below.
In the line where the split is done by a colon and you access position 2, not all returns from the rfapi.upload_image
have the status code, and so they don't have this second colon.
@caiquejjx @LinasKo @tonylampada
This PR should fix that, and another things:
Description
Fixes issue #235 by raising
UploadError
exceptions that happens insidesingle_upload
Type of change
Please delete options that are not relevant.
How has this change been tested, please provide a testcase or example of how you tested the change?
Same test as the issue
Any specific deployment considerations
For example, documentation changes, usability, usage/costs, secrets, etc.
Docs