Refactor way of working with RQ meta #9082
Conversation
|
❌ Some checks failed |
|
❌ Some checks failed |
Marishka17
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #9082 +/- ##
===========================================
+ Coverage 73.18% 73.23% +0.05%
===========================================
Files 446 447 +1
Lines 45661 45833 +172
Branches 3907 3907
===========================================
+ Hits 33418 33568 +150
- Misses 12243 12265 +22
|
Marishka17
changed the title
Marishka17
force-pushed
the
mk/refactor_working_with_rq_meta
branch
from
3348a62 to
7ea0523
Compare
Marishka17
changed the title
Marishka17
force-pushed
the
mk/refactor_working_with_rq_meta
branch
from
b99d1b2 to
9ce85ac
Compare
Marishka17
changed the title
| def to_dict(self): | ||
| return self.meta |
There was a problem hiding this comment.
Okay, I also think so, but I made this way because:
ImmutableRQMetaAttributeexpects objects with ametaattribute and I would not prefer to useobj.to_dict()[SomeField]- in some places we need to have the "dict" representation of an user or request (
BaseRQMeta.for_job(rq_job).user.to_dict()). In such cases, I also don't like callingBaseRQMeta.for_job(rq_job).user.metabecause it is less informative.
So, I keptto_dictonly forUserMetaandRequestMeta.
cvat/apps/engine/rq.py
Outdated
|
|
||
| @classmethod | ||
| def for_job(cls, job: RQJob): | ||
| return cls(job=job) |
There was a problem hiding this comment.
| return cls(job=job) | |
| return cls(job=job, meta=job.meta) |
This would simplify the rest of the class: no need for the if in def meta, no need for the assert in __init__.
There was a problem hiding this comment.
no need for the assert in init
frankly speaking, we must check that job.meta and meta refer to the same object in that case
cvat/apps/engine/rq.py
Outdated
|
|
||
| user = request.user | ||
|
|
||
| return cls.for_meta( |
There was a problem hiding this comment.
Isn't the class redundant here? You're creating a dictionary, wrapping it, then immediately unwrapping it.
There was a problem hiding this comment.
Okay, right now there is no reason to do so. In the previous implementation, I thought it would be better to check that the generated meta matches the expected structure.
cvat/apps/engine/rq.py
Outdated
| self, *, job: RQJob | None = None, meta: dict[RQJobMetaField, Any] | None = None | ||
| ) -> None: | ||
| assert (job and not meta) or (meta and not job), "Only job or meta can be passed" | ||
| def __init__(self, *, job: RQJob | None = None, meta: dict[str, Any] | None = None) -> None: |
There was a problem hiding this comment.
| def __init__(self, *, job: RQJob | None = None, meta: dict[str, Any] | None = None) -> None: | |
| def __init__(self, *, job: RQJob | None = None, meta: dict[str, Any]) -> None: |
Since you've updated for_job, meta is always passed, so you don't have to support it being missing anymore.
Marishka17
force-pushed
the
mk/refactor_working_with_rq_meta
branch
from
f10eff1 to
227c877
Compare
|
Motivation and context
This PR changes the way we work with RQ meta. The previous implementation tried to standardize the fields saved in the meta and how to access them, however, we had to remember the structure of nested objects. This approach explicitly defines the structure of the meta being used.
Extracted from #9075
Partially depends on #9077
How has this been tested?
Checklist
developbranchLicense
Feel free to contact the maintainers if that's a concern.