[do not merge] Reusable requests functionality #9230
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| from django.conf import settings | ||
|
|
||
|
|
||
| class LayeredKeyDict(dict): |
There was a problem hiding this comment.
IMO, using a custom class here obscures the underlying logic. I would suggest just adding an ordinary helper function:
def get_queue_for_triplet(action, target, subresource):
if queue := MAPPING.get((action, target, subresource)):
return queue
if queue := MAPPING.get((action, subresource)):
return queue
return MAPPING[action]There was a problem hiding this comment.
I prefer to keep my implementation because using get_queue_for_triplet requires passing action, target, and subresource every time, but in most cases, just the action is enough to determine the corresponding queue.
Moreover, I want to retain the flexibility of accessing the mapping via ACTION_TO_QUEUE[action] or ACTION_TO_QUEUE[(action, subresource)], since the QUEUE_SELECTORS defined in the RequestId subclasses are used to determine the queue based on the known RequestIdSubclass.
So, the pipeline looks like this:
- There are several RequestId subclasses. Based on
RQ_QUEUES/PARSED_JOB_ID_CLASS/ and each RequestId subclass that hasQUEUE_SELECTORS, we build theACTION_TO_QUEUE(queue_selector -> queue). ACTION_TO_QUEUEis used to determine the queue based on the parsed action, target, and subresource.ACTION_TO_QUEUEis also used when the legacy rq_id is passed. In that case, we know the subclass, but we need to determine the corresponding queue, so one of the subclass'sQUEUE_SELECTORSis used.
There was a problem hiding this comment.
I think everything you've described could be achieved by changing the signature to get_queue_to_selector(action, subresource=None, target=None). But since this is just a stylistic disagreement, I'm not going to insist.
cvat/apps/redis_handler/rq.py
Outdated
| queue_is_known = bool(queue) | ||
|
|
||
| if not queue_is_known: | ||
| _parsed = _RequestIdForMapping(**params) |
There was a problem hiding this comment.
What's the purpose of this? Couldn't we just use params["action"], etc. in the following line?
There was a problem hiding this comment.
What's the purpose of this?
To run validations defined in the RequestId class, but okay, I've removed the helper class
Marishka17
force-pushed
the
mk/reusable_requests_functionality
branch
from
ecb133b to
620b81b
Compare
| @@ -2104,35 +2097,33 @@ def test_api_v2_export_import_dataset(self): | |||
| ]: | |||
| # TO-DO: fix bug for this formats | |||
| continue | |||
| if upload_format_name == "Ultralytics YOLO Classification 1.0": | |||
There was a problem hiding this comment.
Why did this workaround need to be added now? Your changes don't seem like they should impact whether the import succeeds or fails.
There was a problem hiding this comment.
They impact because now we check that the operation is finished successfully (by default).
Before that, there was a check that the import "initialization" (I remember about sync mode) response was 202.
cvat/cvat/apps/engine/views.py
Line 3445 in 06b9aac
|
|
||
| @classmethod | ||
| def parse_filename( | ||
| cls, filename: str, | ||
| ) -> ParsedDatasetFilename | ParsedBackupFilename: | ||
| ) -> ParsedExportFilename | ParsedExportFilenameWithConstructedId: |
There was a problem hiding this comment.
This return type is equivalent to just ParsedExportFilename.
There was a problem hiding this comment.
I think it isn't, because they have different types for the file_id attribute
| where rq_id is obtained from the response of the initializing request. | ||
| """)) | ||
|
|
||
| class DeprecatedResponse(Response): |
There was a problem hiding this comment.
Doesn't seem like a custom class is needed here - this could just be a function returning a Response.
There was a problem hiding this comment.
Are you talking about a function that accepts the same args as the Response constructor? In that case, what are the benefits of such an approach? The DeprecatedResponse does not introduce a lot of logic, but it looks like we still work with the DRF Response. IMHO, return DeprecatedResponse(msg, status=..., headers=) looks better than return get_deprecated_response(msg, status=..., headers=...)
Co-authored-by: Roman Donchenko <[email protected]>
Co-authored-by: Roman Donchenko <[email protected]>
|
Motivation and context
API changes:
POST /api/consensus/merges?rq_id=rq_idreturns 410 status code, this endpoint no longer supports process status checkingGET /api/projects/id/dataset?action=import_statusreturns 410 status code, this endpoint no longer supports process status checkingPOST /api/projects/backup?rq_id=rq_idreturns 410 status code, this endpoint no longer supports process status checkingPOST /api/tasks/backup?rq_id=rq_idreturns 410 status code, this endpoint no longer supports process status checkingPUT /api/tasks/id/annotations?rq_id=rq_id&format=formatreturns 410 status code, this endpoint no longer supports process status checkingPUT /api/jobs/id/annotations?rq_id=rq_id&format=formatreturns 410 status code, this endpoint no longer supports process status checkingGET /api/eventsis deprecated in favor of the following API:POST /api/events/export(returns 202 with Request ID)GET /api/events/download?rq_id=rq_id(private endpoint, should be used only as result_url)POST /api/quality/reports/rq_id=rq_idis deprecated in favor ofGET /api/requests/rq_idArchitecture visible changes:
/data/cache/export/instead of/data/tmp/. The_clear_export_cachefunction is deleted, since cache files are deleted one day after creation (by default) by thecleanup_export_cache_directorycron job.API backward-incompatible changes:-POST /api/projects|tasks|jobs/id/annotations|dataset|backupAPI accepts unified body:{"file": ...}when file is uploaded now via TUS.SDK backward-incompatible changes:
AnnotationFileRequestProjectFileRequestDatasetFileRequestTaskFileRequestDatasetWriteRequest/BackupWriteRequest/TaskAnnotationsWriteRequest;JobAnnotationsUpdateRequest/TaskAnnotationsUpdateRequestRqIdrenamed toRequestIdList with all possible request types:
How has this been tested?
Checklist
developbranchLicense
Feel free to contact the maintainers if that's a concern.