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

Importing annotations without deleting current ones #4747

Open
2 tasks done
domef opened this issue Jul 13, 2022 · 19 comments
Open
2 tasks done

Importing annotations without deleting current ones #4747

domef opened this issue Jul 13, 2022 · 19 comments
Assignees
Labels
Easy to fix The issue is easy to fix and probably it will be release in a next minor release enhancement New feature or request good first issue Good for newcomers

Comments

@domef
Copy link

domef commented Jul 13, 2022

My actions before raising this issue

When importing annotations into a task, CVAT deletes current annotations. Is there a way to keep them and have both sets of annotations in the same task?

@bsekachev bsekachev added enhancement New feature or request Easy to fix The issue is easy to fix and probably it will be release in a next minor release labels Aug 21, 2022
@ArtMarciano
Copy link

Im having the same issue. Was it resolved?

@bsekachev bsekachev added the good first issue Good for newcomers label Mar 11, 2024
@OmarHassan2334
Copy link

Can you describe the problem in more detail like what web browser you're using and what step you took for the issue to occur, also please share images to show the problem better.

@OmarHassan2334
Copy link

Hey @nmanovic, I was also able to replicate the error and would like to have a go at it.

1 similar comment
@OmarHassan2334
Copy link

Hey @nmanovic, I was also able to replicate the error and would like to have a go at it.

@nmanovic
Copy link
Contributor

@OmarHassan2334 , could you please send me an email with a screenshot of debugging of CVAT? I cannot locate it. If you read instructions, only after you confirm that you setup the development environment, I can assign an issue.

@EBayego
Copy link

EBayego commented Mar 30, 2024

Hi @nmanovic, I will take on this other issue to solve it

@zhiltsov-max
Copy link
Contributor

zhiltsov-max commented Apr 4, 2024

Hi, I wouldn't call it's an "easy" problem to solve, nor would I call it a bug or an error. It's implemented this way on purpose - so that after importing annotations you had only what you just uploaded. Though I agree, it can be improved by allowing to keep existing annotations. This is handled by the server, not UI. Thus, the nice solution would be:

  • to add a switch in UI annotation import dialog
  • to add a server API flag (e.g. a query parameter) for keeping old annotations on uploading (should be enabled by default)
  • forward the flag from the API all the way up to this place
  • add corresponding tests

This should be done both for tasks and jobs as all of them allow annotation uploading.

@EBayego
Copy link

EBayego commented Apr 10, 2024 via email

@zhiltsov-max
Copy link
Contributor

@EBayego, I hope you feel better now. For API please check cvat/apps/*/views.py, mostly engine.views. An example of a parameter is conv_mask_to_poly.

@EBayego
Copy link

EBayego commented Apr 10, 2024

@zhiltsov-max
Yes, I'm feeling better now, thank you!
If I'm not mistaken, the parameter is retrieved from a query using request.query_params.get(), but I can't find where it's created. Could you give me a quick guide of how the flow works? I'm sorry for needing this much help. Thank you!

@zhiltsov-max
Copy link
Contributor

@EBayego, it's parsed by Django automatically. As you can see in the example above, you can add an extra API documentation so that it was shown in OpenAPI schema or swagger (check http://localhost:8080/api/swagger/ and http://localhost:8080/api/docs/).

@EBayego
Copy link

EBayego commented Apr 13, 2024

Hi, I'm facing I suppose the last problem. I'm trying to maintain the current annotations by creating new shapes (and probably tags and tasks too) with the same data, but letting django create new ids for them in order to avoid errors. But even with that, I'm getting the following error:
image

I'm only creating new ids for those who already exists, by checking the list that contains current shapes. models.LabeledShape.objects.values_list('id', flat=True). Any idea on how to deal with this?

Thank you!

@zhiltsov-max
Copy link
Contributor

Hi,

I'm facing I suppose the last problem. I'm trying to maintain the current annotations by creating new shapes (and probably tags and tasks too) with the same data, but letting django create new ids for them in order to avoid errors.

It's certainly doable this way too, but why did you decide to it like this? Did you try to comment the annotation removal line?

About the error - are you sure it's about annotation ids? It's hard to say for sure without the stack trace, please try to check it via docker logs cvat_worker_import.

@EBayego
Copy link

EBayego commented Apr 14, 2024

It's certainly doable this way too, but why did you decide to it like this? Did you try to comment the annotation removal line?

Yes, you are right, that's the option I tried at first, but I encountered an error which I attempted to resolve in this way. However, I am no longer able to reproduce it in any way, so it was likely my mistake in the import file format or in managing labels or not saving annotations, i don't really know.

Finally, regarding the tests, I simply modified the lines where it calls import_task_annotations by adding a final True corresponding to the new flag I created. Should I create any more tests? Thank you very much for your help and time, I appreciate it!

@zhiltsov-max
Copy link
Contributor

@EBayego, we're trying to move away from using the django-based tests. Please add a test in the tests/python/rest_api tests.

@EBayego
Copy link

EBayego commented Apr 15, 2024

Okay, in that case, I believe that this existing test, if runs successfully, covers the development I have done, as I have only included a parameter in the internal query, the same case as conv_mask_to_poly. I will prepare the pull request following your documentation. If there are any changes or suggestions, let me know!

@zhiltsov-max
Copy link
Contributor

zhiltsov-max commented Apr 15, 2024

I believe that this existing test, if runs successfully, covers the development I have done, as I have only included a parameter in the internal query, the same case as conv_mask_to_poly.

I'm not sure I got what you mean here. In this PR there is a new API option (actually 2 - for tasks and for jobs) to allow keeping the old annotations during the import. In needs to be tested. There is no way it could be covered by any existing test, as it didn't exist before this PR.

@EBayego
Copy link

EBayego commented Apr 15, 2024

I assumed that this was a parameter with the same functionality as the one you gave me as an example, conv_mask_to_poly. In the Swagger documentation, this parameter does not appear:
image

And in the code, therefore, it also does not appear as an OpenApiParameter. However, it is taken from the query parameters within the method here. I assumed that either the Swagger documentation had not been updated, or that it was simply an internal parameter relevant only for specific queries, and the parameter was just a part of 'options', which is a list sent along with other options, in this case only conv_mask_to_poly from here.

If it needs to be added to the Swagger documentation, then I imagine the test I need to perform is similar to this one, but adding the part of passing more parameters to the query and not just the file format and the file to import.

Since I don't see direct examples to follow and save time, it will take me more time to figure out how to write the appropriate tests. And given that today, if I'm not mistaken, is the last day to have a PR ready, I will submit the PR to document what I've done so far, and I will continue with the development in the coming days.

@zhiltsov-max
Copy link
Contributor

In the Swagger documentation, this parameter does not appear

Most of the OpenAPI parameters must be added manually with the decorator, except regular filtering parameters and several others. conv_mask_to_poly was supposed to be a temporary solution, and it can be considered deprecated, so we don't want to advertise it in the API.

EBayego pushed a commit to EBayego/cvat that referenced this issue Apr 15, 2024
Keep current annotations without deleting them, adding the imported ones.
EBayego pushed a commit to EBayego/cvat that referenced this issue Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Easy to fix The issue is easy to fix and probably it will be release in a next minor release enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

7 participants