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

Simple Review Pipeline (Server) #2338

Merged
merged 40 commits into from
Dec 1, 2020
Merged

Simple Review Pipeline (Server) #2338

merged 40 commits into from
Dec 1, 2020

Conversation

bsekachev
Copy link
Member

@bsekachev bsekachev commented Oct 21, 2020

Motivation and context

The pull request introduces simple validation pipeline to annotations quality control and some basic features for that
Screenshot from 2020-10-21 09-52-46

New REST API endpoints:
DELETE /api/v1/reviews/<review_id>
POST /api/v1/reviews

DELETE /api/v1/issues/<issue_id>
GET /api/v1/issues/<issue_id>/comments
PATCH /api/v1/issues/<issue_id>

POST /api/v1/comments
PATCH /api/v1/comments/<comment_id>
DELETE /api/v1/comments/<comment_id>

GET /api/v1/jobs/<job_id>/issues
GET /api/v1/jobs/<job_id>/reviews

How has this been tested?

Not tested yet

Checklist

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.
  • I have updated the license header for each file (see an example below)
# Copyright (C) 2020 Intel Corporation
#
# SPDX-License-Identifier: MIT

@bsekachev bsekachev added the enhancement New feature or request label Oct 21, 2020
@bsekachev bsekachev requested a review from nmanovic as a code owner October 21, 2020 06:59
@coveralls
Copy link

coveralls commented Oct 21, 2020

Coverage Status

Coverage increased (+0.4%) to 63.547% when pulling 3cdaf1e on bs/review_pipeline into 6ef3ff2 on develop.

@bsekachev bsekachev changed the title [WIP] Simple Validation Pipeline Simple Review Pipeline Oct 26, 2020
@bsekachev bsekachev changed the title Simple Review Pipeline Simple Review Pipeline (Server) Oct 26, 2020
@Loc-Vo
Copy link

Loc-Vo commented Nov 2, 2020

May I know when this feature will be available?
Is there any way I can try this feature?

@bsekachev
Copy link
Member Author

May I know when this feature will be available?
Is there any way I can try this feature?

You can try the feature building cvat from the branch bs/review_pipeline_2

cvat/apps/engine/models.py Outdated Show resolved Hide resolved
@nmanovic
Copy link
Contributor

@bsekachev , let's discuss REST API design principles which are described in the PR: #2427. I will vote to finalize them and use in the PR.

  • DELETE /api/v1/reviews/<review_id>. It is OK.

  • DELETE /api/v1/issues/<issue_id>. It is OK

  • POST /api/v1/issues/<issue_id>/comments/create. It should be POST /api/v1/comments.

  • PATCH /api/v1/issues/<issue_id>/resolve. It should be PATCH /api/v1/issues/<issue_id>.

  • PATCH /api/v1/issues/<issue_id>/unresolve. It should be PATCH /api/v1/issues/<issue_id>.

  • GET /api/v1/issues/<issue_id>/comments. It should be GET /api/v1/comments?issue=id (but it is OK with the current REST API design).

  • PUT /api/v1/comments/<comment_id>. Do you really need PUT or PATCH is enough?

  • PATCH /api/v1/comments/<comment_id>. It is OK.

  • DELETE /api/v1/comments/<comment_id>. It is OK.

  • GET /api/v1/jobs/<job_id>/issues. It should be GET /api/v1/issues?job=id (but it is OK with the current REST API design).

  • GET /api/v1/jobs/<job_id>/reviews. It should be GET /api/v1/reviews?job=id (but it is OK with the current REST API design).

  • GET /api/v1/jobs/<job_id>/reviews/summary. Probably it should be removed.

  • POST /api/v1/jobs/<job_id>/reviews/create. It should be POST /api/v1/reviews.

@bsekachev
Copy link
Member Author

@nmanovic

PUT /api/v1/comments/<comment_id>. Do you really need PUT or PATCH is enough?

Actually it was auto generated with mixins.UpdateModelMixin

GET /api/v1/jobs/<job_id>/reviews/summary

Do not we need to have review summary feature on UI? I explained why we should not perform this logic client-side requesting all the reviews above. Or do you believe it doesn't matter?

Will try to fix others.

@nmanovic
Copy link
Contributor

@nmanovic

PUT /api/v1/comments/<comment_id>. Do you really need PUT or PATCH is enough?

Actually it was auto generated with mixins.UpdateModelMixin

Need to have only PATCH in the case. Please find a workaround. I believe in the past I already did something similar. Probably you can list which methods are supported by a view.

@bsekachev
Copy link
Member Author

@nmanovic

Updated, new list of methods in description.

@chaoyifei
Copy link

HI, when this feature will be available

@bsekachev
Copy link
Member Author

@nmanovic Can we merge the PR?

@nmanovic
Copy link
Contributor

nmanovic commented Dec 1, 2020

@bsekachev , I will do that today if I don't see any other issues.

@nmanovic nmanovic merged commit f2fb053 into develop Dec 1, 2020
@bsekachev bsekachev deleted the bs/review_pipeline branch December 1, 2020 14:00
@Viditagarwal7479
Copy link
Contributor

Viditagarwal7479 commented Mar 25, 2024

#2338 (comment)

Do not we need to have review summary feature on UI? I explained why we should not perform this logic client-side requesting all the reviews above. Or do you believe it doesn't matter?

Hi @bsekachev ,
Is this the UI connected to the review pipeline mentioned above?
image

If we consider the above image as the UI for reviewing. The reviewer will go through the image in case they find any issues in an image they will mark the issue and reject the whole job. This might be fine when only one person is reviewing a part of the data (referred as a job in cvat), i.e. not crowdsourced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants