Using dedicated event to store working time #7958
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce a new mechanism for handling and generating working time events in the CVAT application. This involves adding new methods and constants, refactoring existing code for better event handling, and importing necessary modules. The updates enhance the functionality to process client events more effectively and generate corresponding working time events. Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant Client
participant Server
participant Serializer
participant Handler
Client->>Server: Push client events
Server->>Handler: handle_client_events_push(request, data)
Handler->>Serializer: Deserialize events
Serializer->>Handler: Return deserialized events
Handler->>Handler: Process events and generate working time events
Handler->>Server: Return response
Server->>Client: Response with status
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- cvat/apps/events/serializers.py (5 hunks)
Additional Context Used
Ruff (1)
cvat/apps/events/serializers.py (1)
122-122: Do not use bare
except
Additional comments not posted (4)
cvat/apps/events/serializers.py (4)
8-8: Import ofsuppressfromcontextlibis appropriate for handling exceptions where specific errors are expected and can be safely ignored.
38-38: The constant_WORKING_TIME_SCOPEis well-defined and follows naming conventions for constants in Python.
40-70: The method_generate_wt_eventis well-structured and handles the creation of working time events. It uses thesuppresscontext manager to handle cases where aJobmight not exist, which is a good use of Python's capabilities to handle exceptions cleanly.
Line range hint
91-148: The modifications in theto_internal_valuemethod to handle working time per job and generate events accordingly are significant. It's important to ensure that these changes are thoroughly tested, especially since they involve time calculations and JSON operations which are error-prone.Would you like assistance in writing comprehensive unit tests for these changes?
Verification successful
The test functions in
cvat/apps/events/tests/test_events.pycover various scenarios related to event handling, including instant events, compressed events, and events with different gap durations. These tests indirectly verify the functionality of theto_internal_valuemethod and the new working time handling logic.
test_instanttest_compressedtest_instants_with_short_gaptest_instants_with_long_gaptest_compressed_with_short_gaptest_compressed_with_long_gaptest_compressed_containedtest_compressed_overlappingtest_instant_inside_compressedtest_previous_instant_short_gaptest_previous_instant_long_gaptest_previous_compressed_short_gaptest_previous_compressed_long_gapThese tests ensure that the working time calculations and event processing logic are functioning correctly.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new working time handling logic integrates well with existing event processing. # Test: Search for unit tests related to `to_internal_value` method. Expect: Tests covering the new logic. rg --type python $'def test_to_internal_value'Length of output: 78
Script:
#!/bin/bash # Description: Verify that the new working time handling logic integrates well with existing event processing. # Test: Search for unit tests related to `to_internal_value` method. Expect: Tests covering the new logic. rg --type py 'def test_to_internal_value'Length of output: 43
Script:
#!/bin/bash # Description: Verify that the new working time handling logic integrates well with existing event processing. # Test: Search for references to `ClientEventsSerializer` in test files to identify any relevant tests. rg --type py 'ClientEventsSerializer' --glob '*test*.py'Length of output: 808
Script:
#!/bin/bash # Description: Verify that the new working time handling logic integrates well with existing event processing. # Test: Search for test functions within the `test_events.py` file and inspect their content for coverage of the `to_internal_value` method. rg --type py 'def ' cvat/apps/events/tests/test_events.pyLength of output: 886
Script:
#!/bin/bash # Description: Extract the content of test functions within the `test_events.py` file to verify coverage of the `to_internal_value` method. # Extract the content of the test functions. rg --type py -A 10 'def test_' cvat/apps/events/tests/test_events.pyLength of output: 4890
bsekachev
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- cvat/apps/events/serializers.py (4 hunks)
Additional Context Used
Ruff (1)
cvat/apps/events/serializers.py (1)
122-122: Do not use bare
except
Additional comments not posted (3)
cvat/apps/events/serializers.py (3)
8-8: Ensure thesuppressimport is used appropriately.This import is correctly used in the context of suppressing exceptions when a job does not exist in the database.
38-38: Define the scope constant clearly.The
_WORKING_TIME_SCOPEconstant is well-defined and clearly indicates its purpose, which is to tag events related to working time.
40-70: Review the method_generate_wt_eventfor correctness and efficiency.This method correctly calculates the working time and creates an event. The use of
suppressto handle the potentialJob.DoesNotExistexception is appropriate. However, consider adding more detailed logging for debugging purposes.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- cvat/apps/events/serializers.py (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- cvat/apps/events/serializers.py
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- cvat/apps/events/serializers.py (4 hunks)
- cvat/apps/events/views.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- cvat/apps/events/serializers.py
Additional Context Used
Ruff (1)
cvat/apps/events/views.py (1)
8-8:
rest_framework.exceptions.PermissionDeniedimported but unused
cvat/apps/events/views.py
Outdated
| @@ -5,6 +5,7 @@ | |||
| from django.conf import settings | |||
| from rest_framework import status, viewsets | |||
| from rest_framework.response import Response | |||
| from rest_framework.exceptions import PermissionDenied | |||
There was a problem hiding this comment.
Remove unused import.
- from rest_framework.exceptions import PermissionDeniedThis import statement for PermissionDenied is not used anywhere in the file. It should be removed to keep the code clean and efficient unless there are plans to use it in the near future.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| from rest_framework.exceptions import PermissionDenied |
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- cvat/apps/events/handlers.py (3 hunks)
- cvat/apps/events/serializers.py (2 hunks)
- cvat/apps/events/views.py (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- cvat/apps/events/views.py
Additional Context Used
Ruff (1)
cvat/apps/events/handlers.py (1)
42-42:
.serializers.ClientEventsSerializerimported but unused
Additional comments not posted (4)
cvat/apps/events/serializers.py (3)
45-48: Ensure correct handling of time correction for previous events.
52-54: Proper exception handling for JSON payload parsing.
57-64: Ensure that the updated event data includes all necessary fields and that the payload is correctly serialized.cvat/apps/events/handlers.py (1)
602-692: Review the logic inhandle_client_events_pushto ensure it correctly calculates and records working time events. This function is central to the PR's objectives and should be thoroughly tested.Verification successful
The
handle_client_events_pushfunction is called within thecreatemethod of theEventsViewSetclass. Theserializer.validated_datais passed to this function, which includes the client events data.The logic in
handle_client_events_pushappears to be correctly structured to calculate and record working time events based on the provided data. The function processes the events, calculates the working time, and generates the appropriate events to be recorded.
cvat/apps/events/views.py
EventsViewSet.createmethod: Callshandle_client_events_pushwithrequestandserializer.validated_data.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct calculation of working time events. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type python $'handle_client_events_push'Length of output: 75
Script:
#!/bin/bash # Description: Verify the correct calculation of working time events. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg 'handle_client_events_push'Length of output: 275
Script:
#!/bin/bash # Description: Extract relevant lines from cvat/apps/events/views.py to verify the usage of handle_client_events_push. # Extract lines where handle_client_events_push is imported and used. rg -A 5 -B 5 'handle_client_events_push' cvat/apps/events/views.pyLength of output: 930
Script:
#!/bin/bash # Description: Extract the definition of the method in EventsViewSet where handle_client_events_push is called. # Extract the method definition from cvat/apps/events/views.py. rg -A 20 -B 5 'handle_client_events_push' cvat/apps/events/views.pyLength of output: 2754
cvat/apps/events/handlers.py
Outdated
| @@ -36,6 +39,7 @@ | |||
| from cvat.apps.organizations.models import Membership, Organization, Invitation | |||
| from cvat.apps.organizations.serializers import OrganizationReadSerializer, MembershipReadSerializer, InvitationReadSerializer | |||
|
|
|||
| from .serializers import ClientEventsSerializer, EventSerializer | |||
There was a problem hiding this comment.
Remove unused import for ClientEventsSerializer.
- from .serializers import ClientEventsSerializer, EventSerializer
+ from .serializers import EventSerializerCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| from .serializers import ClientEventsSerializer, EventSerializer | |
| from .serializers import EventSerializer |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #7958 +/- ##
========================================
Coverage 83.42% 83.42%
========================================
Files 383 383
Lines 40238 40267 +29
Branches 3772 3772
========================================
+ Hits 33567 33592 +25
- Misses 6671 6675 +4
|
Motivation and context
working_timein general leads to low performance in Clickhouse requests. This patch will not fix it right now, but with this patch, after a period of time we may switch to new quick approach to calculate working time.send:working_timeevents, we may store this scope of events for a longer time (e.g. 5 years instead of one by default).click:elementorsend:exception, ordebug:infoseems not logical.job_id,task_id, etc.Resolved #7884
How has this been tested?
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Security