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

Fix /api/tasks leads to 500 server error #5700

Merged
merged 37 commits into from Feb 24, 2023
Merged

Fix /api/tasks leads to 500 server error #5700

merged 37 commits into from Feb 24, 2023

Conversation

yasakova-anastasia
Copy link
Contributor

@yasakova-anastasia yasakova-anastasia commented Feb 14, 2023

Motivation and context

  • Fixed re-deleting of skeleton sublabels (attempt to remove already removed sublabels)

Screenshot from 2023-02-13 10-00-03

  • Updated Label model (added project to unique_together)
  • Added migration to remove or rename wrong labels

How has this been tested?

REST API tests

Checklist

  • I submit my changes into the develop branch
  • I have added a description of my changes into the CHANGELOG file
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • I have linked related issues (see GitHub docs)
  • I have increased versions of npm packages if it is necessary
    (cvat-canvas,
    cvat-core,
    cvat-data and
    cvat-ui)

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.

@yasakova-anastasia yasakova-anastasia changed the title Fix removal of skeleton sublabels [WIP] Fix removal of skeleton sublabels Feb 14, 2023
@yasakova-anastasia yasakova-anastasia changed the title [WIP] Fix removal of skeleton sublabels Fix /api/tasks leads to 500 server error Feb 15, 2023
@yasakova-anastasia yasakova-anastasia marked this pull request as draft February 15, 2023 13:37
@yasakova-anastasia yasakova-anastasia marked this pull request as ready for review February 15, 2023 16:50
Copy link
Contributor

@zhiltsov-max zhiltsov-max left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to check anything in the existing DB or add a migration to fix existing labels?

cvat/apps/engine/serializers.py Fixed Show resolved Hide resolved
cvat/apps/engine/serializers.py Fixed Show fixed Hide fixed
@yasakova-anastasia yasakova-anastasia marked this pull request as draft February 21, 2023 19:21
cvat/apps/engine/models.py Fixed Show fixed Hide fixed
cvat/apps/engine/models.py Fixed Show fixed Hide fixed
@yasakova-anastasia yasakova-anastasia marked this pull request as ready for review February 22, 2023 08:01
Copy link
Contributor

@zhiltsov-max zhiltsov-max left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add @transaction.atomic to the update_label() too.

@yasakova-anastasia
Copy link
Contributor Author

yasakova-anastasia commented Feb 22, 2023

Please add @transaction.atomic to the update_label() too.

Added.

I noticed that if the project creation fails with a unique labels error, the project is still created (without labels). I'm not sure this is correct. I think in this case the project should not be created. So I suggest adding @transaction.atomic to the create and update methods for the task and project serializers. What do you think?

@zhiltsov-max
Copy link
Contributor

zhiltsov-max commented Feb 22, 2023

This looks like the issue (#5750). Yes, this definitely looks invalid to me. I agree, we should add it, and also check if we can benefit from using ATOMIC_REQUESTS (it's stated that it can reduce performance). We can use it at least for writing requests.

@yasakova-anastasia
Copy link
Contributor Author

I added @transaction.atomic. I think we can replace this with ATOMIC_REQUESTS in another PR.

@nmanovic nmanovic merged commit 8dc272f into develop Feb 24, 2023
@nmanovic nmanovic deleted the ay/skeleton-fixes branch February 24, 2023 08:32
mikhail-treskin pushed a commit to retailnext/cvat that referenced this pull request Jul 1, 2023
- Fixed re-deleting of skeleton sublabels (attempt to remove already
removed sublabels)

![Screenshot from 2023-02-13
10-00-03](https://user-images.githubusercontent.com/49231913/218693868-0afcde6a-935a-4d3f-bfa4-c1793a367f90.png)

- Updated Label model (added `project` to `unique_together`)
- Added migration to remove or rename wrong labels
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants