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

Fix random_per_job option on create job page #8623

Merged
merged 6 commits into from
Nov 2, 2024
Merged

Conversation

klakhov
Copy link
Contributor

@klakhov klakhov commented Oct 31, 2024

Motivation and context

Newly introduced random_per_job option for frame selection method doesnt work properly on create job page. This patch fixes it.
Resolve #8599

How has this been tested?

Checklist

  • I submit my changes into the develop branch
  • I have created a changelog fragment
  • [ ] I have updated the documentation accordingly
  • [ ] I have added tests to cover my changes
  • I have linked related issues (see GitHub docs)
  • I have increased versions of npm packages if it is necessary
    (cvat-canvas,
    cvat-core,
    cvat-data and
    cvat-ui)

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.

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced job initialization flexibility by allowing optional properties.
    • Improved job creation process with structured handling of job data.
  • Bug Fixes

    • Adjusted loading spinner display logic for better user feedback during task fetching.
  • Documentation

    • Updated copyright notice to reflect the year 2024.
  • Style

    • Standardized property naming conventions across job-related interfaces for clarity.

@klakhov klakhov requested a review from bsekachev as a code owner October 31, 2024 12:26
Copy link
Contributor

coderabbitai bot commented Oct 31, 2024

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The changes in this pull request involve modifications to type definitions and function implementations related to job creation and management in the CVAT application. The InitializerType is updated to allow more flexible initialization of job instances. The createJobAsync function now returns a promise resolving to a Job object, and the handling of job data is refined. Additionally, the CreateJobPage and JobForm components have been adjusted for improved loading state management and standardized property naming conventions, respectively.

Changes

File Path Change Summary
cvat-core/src/session.ts Updated InitializerType from Readonly<Omit<SerializedJob, 'labels'> & { labels?: SerializedLabel[] }> to Readonly<Partial<Omit<SerializedJob, 'labels'> & { labels?: SerializedLabel[] }>>.
cvat-ui/src/actions/jobs-actions.ts Changed return type of createJobAsync to ThunkAction<Promise<Job>>. Refined job data handling in createJobAsync.
cvat-ui/src/components/create-job-page/create-job-page.tsx Updated copyright year, consolidated Task import, and improved loading spinner logic in useEffect.
cvat-ui/src/components/create-job-page/job-form.tsx Updated JobDataMutual, JobData, and JobFormData interfaces for property name consistency. Refined job form logic with new state and helper functions.

Assessment against linked issues

Objective Addressed Explanation
Random per job mode is available on the job creation page (8599)

🐰 In the world of jobs, we now see,
A frame selection method, wild and free!
With properties named just right,
Our job creation takes flight!
So hop along, let’s create with glee,
For every task, there’s a job for thee! 🌟


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@klakhov klakhov added the ui/ux label Oct 31, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 the finally block, 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_job option, 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

📥 Commits

Reviewing files that changed from the base of the PR and between 16d4d75 and b177167.

📒 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:

  1. It defines FrameSelectionMethod.RANDOM_PER_JOB as an option
  2. Handles frame selection logic differently based on the method:
    • For RANDOM: uses frameCount parameter
    • For RANDOM_PER_JOB: uses framesPerJobCount parameter
  3. 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()
  4. 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_job method:
    • 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 FrameSelectionMethod correctly defines RANDOM and RANDOM_PER_JOB options
    • Job creation properly handles both methods in the form submission
    • Correct parameter passing through Redux actions to the backend
  • 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.

cvat-ui/src/components/create-job-page/job-form.tsx Outdated Show resolved Hide resolved
@klakhov klakhov requested a review from nmanovic as a code owner October 31, 2024 13:00
@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.30%. Comparing base (ceba1b7) to head (e09005f).

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     
Components Coverage Δ
cvat-ui 78.64% <100.00%> (-0.01%) ⬇️
cvat-server 70.58% <ø> (ø)

Copy link
Member

@bsekachev bsekachev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

sonarqubecloud bot commented Nov 2, 2024

@klakhov klakhov merged commit 7ce5452 into develop Nov 2, 2024
34 checks passed
@cvat-bot cvat-bot bot mentioned this pull request Nov 11, 2024
@bsekachev bsekachev deleted the kl/fix-create-job branch December 3, 2024 06:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

random_per_job frame selection mode doesn't work on the job creation page
3 participants