Fix random_per_job option on create job page
#8623
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 in this pull request involve modifications to type definitions and function implementations related to job creation and management in the CVAT application. The Changes
Assessment against linked issues
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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
🧹 Outside diff range and nitpick comments (5)
cvat-ui/src/components/create-job-page/create-job-page.tsx (1)
Line range hint
24-45: Consider improving error state handling.The loading state management could be improved. Currently,
setFetchingTask(false)is called in thefinallyblock, which means the loading state ends even if the task fetch fails. This could lead to attempting to render the JobForm with a null taskInstance.Consider this alternative implementation:
useEffect((): void => { if (Number.isInteger(id)) { core.tasks.get({ id }) .then(([task]: Task[]) => { if (isMounted() && task) { setTaskInstance(task); + setFetchingTask(false); } }).catch((error: Error) => { if (isMounted()) { notification.error({ message: 'Could not fetch requested task from the server', description: error.toString(), }); + setFetchingTask(false); } - }).finally(() => { - if (isMounted()) { - setFetchingTask(false); - } }); } else { notification.error({ message: 'Could not receive the requested task from the server', description: `Requested task id "${id}" is not valid`, }); setFetchingTask(false); } }, []);cvat-ui/src/components/create-job-page/job-form.tsx (3)
64-70: Consider extracting the frame count logic to a helper function.While the conditional spread operator works correctly, extracting this logic would improve readability and maintainability.
+ const getFrameCountData = (method: FrameSelectionMethod, count: number) => + method === FrameSelectionMethod.RANDOM + ? { frameCount: count } + : { framesPerJobCount: count }; const data: JobData = { taskID: task.id, frameSelectionMethod: values.frameSelectionMethod, type: values.type, seed: values.seed, - ...(values.frameSelectionMethod === FrameSelectionMethod.RANDOM ? - { frameCount: values.frameCount } : { framesPerJobCount: values.frameCount } - ), + ...getFrameCountData(values.frameSelectionMethod, values.frameCount), };
121-124: Add error handling to form synchronization.The useEffect hook should handle potential errors during form value retrieval and updates.
useEffect(() => { - const currentQuantity = form.getFieldValue('quantity'); - onQuantityChange(currentQuantity); + try { + const currentQuantity = form.getFieldValue('quantity'); + if (currentQuantity !== null) { + onQuantityChange(currentQuantity); + } + } catch (error) { + console.error('Failed to synchronize form values:', error); + } }, [form, frameSelectionMethod]);
136-138: Enhance form accessibility.While the UI changes work correctly, consider adding ARIA labels and descriptions for better screen reader support.
<Select virtual={false} className='cvat-select-frame-selection-method' onChange={setFrameSelectionMethod} + aria-label="Frame selection method" + aria-description="Choose between random uniform and random per job frame selection" >Also applies to: 157-164, 182-184
cvat-core/src/session.ts (1)
466-466: Consider documenting required fields in InitializerType.While making all fields optional provides flexibility for job initialization, which aligns with fixing the
random_per_joboption, it would be helpful to document which fields are actually required for proper job functionality.Consider adding a JSDoc comment above the type definition:
/** * Type for initializing Job instances * @remarks * Required fields: * - type: JobType * - task_id: number * Other fields are optional and will use default values if not provided */ type InitializerType = Readonly<Partial<Omit<SerializedJob, 'labels'> & { labels?: SerializedLabel[] }>>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
cvat-core/src/session.ts(1 hunks)cvat-ui/src/actions/jobs-actions.ts(1 hunks)cvat-ui/src/components/create-job-page/create-job-page.tsx(3 hunks)cvat-ui/src/components/create-job-page/job-form.tsx(8 hunks)
🔇 Additional comments (10)
cvat-ui/src/components/create-job-page/create-job-page.tsx (3)
Line range hint 1-14: LGTM! Import consolidation improves code organization.
The consolidation of imports from 'cvat-core-wrapper' improves code maintainability.
Line range hint 20-22: Verify state management for random_per_job option.
While the basic task fetching state is handled, we should verify that the random_per_job selection state is properly managed in the JobForm component.
#!/bin/bash
# Search for random_per_job handling in JobForm and related components
echo "Searching for random_per_job handling..."
rg -l "random_per_job" --type ts --type tsx
# Search for frame selection mode state management
echo "Searching for frame selection state management..."
ast-grep --pattern 'const [$_] = useState<{ $$$frameSelection$$$: $_ }>'Line range hint 60-74: Verify JobForm implementation for random_per_job support.
The loading state condition improvement is good, but we need to verify that the JobForm component properly implements the random_per_job frame selection mode as mentioned in issue #8599.
✅ Verification successful
JobForm properly implements random_per_job frame selection mode
The JobForm implementation correctly supports the random_per_job frame selection mode:
- It defines
FrameSelectionMethod.RANDOM_PER_JOBas an option - Handles frame selection logic differently based on the method:
- For
RANDOM: usesframeCountparameter - For
RANDOM_PER_JOB: usesframesPerJobCountparameter
- For
- Adjusts calculations and UI based on the selected method:
- Updates quantity label to "Quantity per job" for random_per_job mode
- Properly handles frame count calculations using
sizeBase()
- Correctly submits the data with appropriate parameters in the
submit()function
The implementation aligns with the requirements from issue #8599.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for JobForm implementation and its frame selection logic
echo "Searching for JobForm implementation..."
rg -A 10 "export.*JobForm" --type tsx
# Search for frame selection mode handling in forms
echo "Searching for frame selection mode handling..."
ast-grep --pattern 'frameSelectionMode.*random_per_job'
Length of output: 411
Script:
#!/bin/bash
# Search for JobForm implementation with correct file extension
echo "Searching for JobForm implementation..."
rg -A 10 "export.*JobForm" --type ts --type tsx --type jsx
# Search for frame selection related code
echo "Searching for frame selection related code..."
rg -A 5 "frameSelection|random_per_job" --type ts --type tsx --type jsx
# Find all files that might contain JobForm
echo "Finding JobForm files..."
fd "job.*form" -e ts -e tsx -e jsx
Length of output: 528
Script:
#!/bin/bash
# Let's examine the job-form.tsx content directly
echo "Examining JobForm implementation..."
cat cvat-ui/src/components/create-job-page/job-form.tsx
# Also search for any frame selection related code using grep instead
echo "Searching for frame selection related code..."
grep -r "frameSelection\|random_per_job" cvat-ui/src/components/
Length of output: 14852
cvat-ui/src/actions/jobs-actions.ts (2)
99-99: LGTM: Improved type safety with Promise return type.
The function signature now correctly indicates that it returns a Promise resolving to a Job object, which provides better type safety and makes the API contract clearer.
100-111: Verify frame selection parameters are properly handled.
The separation of core job data and frame selection parameters looks good. However, let's verify that these parameters are properly handled by the backend API, especially for the random_per_job frame selection method.
✅ Verification successful
Based on the search results, I can now provide a final verification response. The code shows that frame selection parameters are properly handled in both frontend and backend:
Frame selection parameters are properly validated and handled
The backend implementation shows comprehensive validation and handling of frame selection parameters:
- For
random_per_jobmethod:- Validates frames_per_job_count against segment size
- Ensures proper distribution of frames across jobs
- Handles both count and share-based frame selection
- The frontend correctly separates core job data and frame selection parameters
- The parameters are properly passed through the API endpoints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify frame selection method handling in the codebase
# Expected: Find related backend API endpoint and parameter validation
# Test 1: Check backend API endpoint handling for frame selection
rg -A 10 "frame_selection_method.*random_per_job" --type python
# Test 2: Check parameter validation for frame count and frames per job
rg -A 5 "frame_count|frames_per_job_count" --type python
Length of output: 186
Script:
#!/bin/bash
# Let's try searching without file type restriction since python extension wasn't recognized
# Search for frame selection method handling
rg -A 10 "frame_selection_method.*random_per_job"
# Search for frame selection method validation
rg -A 5 "frame_selection_method.*validate"
# Search for frame count and frames per job parameters
rg -A 5 "frame_count|frames_per_job_count"
# Look for API endpoint definitions that might handle these parameters
rg -A 10 "class.*JobViewSet"
Length of output: 225522
cvat-ui/src/components/create-job-page/job-form.tsx (3)
29-30: LGTM: Interface changes follow TypeScript conventions.
The property renames from snake_case to camelCase and the addition of optional frame count properties improve type safety and maintainability.
Also applies to: 36-37, 41-42
53-53: LGTM: State management properly handles frame selection method.
The addition of frameSelectionMethod state and segmentSize destructuring supports the random_per_job functionality.
Also applies to: 58-58
Line range hint 1-266: Verify usage patterns of frame selection methods.
Let's ensure consistent usage of frame selection methods across the codebase.
✅ Verification successful
Frame selection methods are consistently implemented across the codebase
The verification shows that the frame selection methods are properly implemented and used consistently across the codebase:
-
Frontend (React/TypeScript):
- Enum
FrameSelectionMethodcorrectly definesRANDOMandRANDOM_PER_JOBoptions - Job creation properly handles both methods in the form submission
- Correct parameter passing through Redux actions to the backend
- Enum
-
Backend (Python/Django):
- Consistent snake_case naming in API endpoints and models
- Proper validation and handling in serializers
- Comprehensive test coverage for both methods
- Frame count calculations are properly implemented for both random uniform and random per job selections
The implementation maintains consistency between frontend and backend, with proper type checking and validation at both ends.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for frame selection method usage patterns
echo "Checking frame selection method usage..."
rg -l "FrameSelectionMethod" --type ts --type tsx
echo "Checking for potential snake_case usage..."
rg "frame_selection_method" --type ts --type tsx
echo "Checking for frame count calculations..."
rg "(frameCount|framesPerJobCount)" --type ts --type tsx
Length of output: 518
Script:
#!/bin/bash
# Search for frame selection method usage patterns without file type restrictions
echo "Checking frame selection method usage..."
rg -l "FrameSelectionMethod"
echo "Checking for potential snake_case usage..."
rg "frame_selection_method"
echo "Checking for frame count calculations..."
rg "(frameCount|framesPerJobCount)"
# Check for related job creation logic
echo "Checking job creation related code..."
rg "createJob" -A 5
Length of output: 25097
cvat-core/src/session.ts (2)
Line range hint 492-545: LGTM: Constructor safely handles optional fields.
The Job constructor implementation properly handles the optional fields using nullish coalescing operators and provides appropriate default values, making it safe and maintainable.
466-466: Verify required fields are validated.
The type change makes all fields optional, but some fields might be required for proper job functionality. Let's verify that required fields are properly validated in the codebase.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8623 +/- ##
===========================================
- Coverage 74.30% 74.30% -0.01%
===========================================
Files 401 401
Lines 43419 43421 +2
Branches 3951 3951
===========================================
+ Hits 32263 32264 +1
- Misses 11156 11157 +1
|
|
Motivation and context
Newly introduced
random_per_joboption for frame selection method doesnt work properly on create job page. This patch fixes it.Resolve #8599
How has this been tested?
Checklist
developbranch[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Style