[WIP] Reset chunks from cache if oudated #8449
Conversation
WalkthroughThe changes introduce enhancements to the caching mechanism for frame metadata in the application. A new timestamp property tracks the last fetch time of metadata, and a function checks for outdated data, refreshing the cache as necessary. Additionally, modifications to the server proxy functions include returning a new property indicating when frame chunks were last updated. These updates aim to improve data consistency and management across various components of the application. Changes
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 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: 5
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- cvat-core/src/frames.ts (9 hunks)
- cvat-core/src/server-proxy.ts (2 hunks)
- cvat-core/src/server-response-types.ts (1 hunks)
Additional comments not posted (10)
cvat-core/src/server-response-types.ts (1)
462-462: Addition ofchunks_updated_dateis appropriate and consistentThe property
chunks_updated_date: string;is properly added to theSerializedFramesMetaDatainterface, following the existing naming conventions and type usage for date properties in this file.cvat-core/src/frames.ts (7)
19-19: Initialization offetchTimestampproperty is appropriateThe addition of the
fetchTimestampproperty toframeDataCachewill help track when the metadata was last fetched. This is essential for ensuring that outdated cache entries are refreshed timely.
59-59: Addition ofchunksUpdatedDatetoFramesMetaDataIncluding
chunksUpdatedDatein theFramesMetaDataclass allows the system to detect changes in chunk definitions effectively.
74-74: Settingchunks_updated_dateto undefined in initial dataEnsuring
chunks_updated_dateis initialized is important for accurate comparisons later in the code.
139-141: Definition of getter forchunksUpdatedDateThe getter method for
chunksUpdatedDatecorrectly retrieves the value fromdata.chunks_updated_date.
507-507: EnsurefetchTimestampis always initializedIn the calculation of
isOutdated, ifcached.fetchTimestampisundefined, it could lead to unexpected results.Please confirm that
fetchTimestampis always initialized beforerefreshJobCacheIfOutdatedis called. If there is a possibility of it beingundefined, consider initializing it with a default value.
653-653: Initialization offetchTimestampproperty
fetchTimestampis correctly initialized with the current timestamp when creating the cache for a job.
709-709: Type annotation foridparameter ingetDeletedFramesSpecifying the type of
idasnumberenhances type safety and clarity.cvat-core/src/server-proxy.ts (2)
1484-1488: Ensure Type Consistency withSerializedFramesMetaDataThe
getMetafunction now returns an object with an additional propertychunks_updated_date. This property may not be defined in theSerializedFramesMetaDatainterface, which could lead to type inconsistencies or TypeScript compilation errors.Consider updating the
SerializedFramesMetaDatainterface to include thechunks_updated_dateproperty, or adjust the return type to accurately reflect the returned object.You can run the following script to verify if
chunks_updated_dateis included in theSerializedFramesMetaDatatype definition:Verification successful
chunks_updated_dateis Already Defined inSerializedFramesMetaDataThe verification process has confirmed that the
chunks_updated_dateproperty is already included in theSerializedFramesMetaDatainterface definition. This property is defined as a string in the interface, which is consistent with the implementation in thegetMetafunction.
- Location:
cvat-core/src/server-response-types.ts, line 462The concern about potential type inconsistencies or TypeScript compilation errors due to the addition of
chunks_updated_dateis not valid. The interface definition already accounts for this property, ensuring type consistency throughout the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if 'chunks_updated_date' is defined in 'SerializedFramesMetaData' interface. # Test: Search for the interface definition including 'chunks_updated_date'. ast-grep --lang typescript --pattern $'interface SerializedFramesMetaData { $$$ chunks_updated_date: $_ $$$ }'Length of output: 1255
1505-1509: Ensure Type Consistency withSerializedFramesMetaDataThe
saveMetafunction now returns an object with an additional propertychunks_updated_date. Similar to the previous comment, this property may not be declared in theSerializedFramesMetaDatainterface, potentially causing type inconsistencies or TypeScript compilation errors.Please update the
SerializedFramesMetaDatainterface to includechunks_updated_date, or adjust the return type accordingly.You can run the following script to verify if
chunks_updated_dateis included in theSerializedFramesMetaDatatype definition:
| export function getContextImage(jobID: number, frame: number): Promise<Record<string, ImageBitmap>> { | ||
| return new Promise<Record<string, ImageBitmap>>((resolve, reject) => { | ||
| if (!(jobID in frameDataCache)) { | ||
| reject(new Error( | ||
| 'Frame data was not initialized for this job. Try first requesting any frame.', | ||
| )); | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider updating cache refresh logic in getContextImage
As inconsistencies may arise, consider implementing cache refresh logic within getContextImage similar to getFrame.
Add cache validation to ensure context images correspond to the latest frame data.
| // In this case (extremely rare, but nevertheless possible) user may get context images related to another frame | ||
| // - if cache gets outdated after getFrame() call | ||
| // - and before getContextImage() call | ||
| // - and both calls refer to the same fram that is refreshed honeypot frame |
There was a problem hiding this comment.
Typographical error in comment
There is a typo in the comment at line 689.
Apply this diff to correct the typo:
- // - and both calls refer to the same fram that is refreshed honeypot frame
+ // - and both calls refer to the same frame that is refreshedAlso, consider clarifying or removing "honeypot frame" if it's not relevant in this context.
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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // - and both calls refer to the same fram that is refreshed honeypot frame | |
| // - and both calls refer to the same frame that is refreshed |
| // basically the following functions may be affected if job cache is outdated | ||
| // - getFrame | ||
| // - getContextImage | ||
| // - getCachedChunks | ||
| // And from this idea we should call refreshJobCacheIfOutdated from each one | ||
| // Hovewer, following from the order, these methods are usually called | ||
| // it may lead to even more confusing behaviour | ||
| // | ||
| // Usually user first receives frame, then user receives ranges and finally user receives context images | ||
| // In this case (extremely rare, but nevertheless possible) user may get context images related to another frame | ||
| // - if cache gets outdated after getFrame() call | ||
| // - and before getContextImage() call | ||
| // - and both calls refer to the same fram that is refreshed honeypot frame | ||
| // Thus, it is better to only call `refreshJobCacheIfOutdated` from getFrame() | ||
| await refreshJobCacheIfOutdated(jobID); | ||
|
|
There was a problem hiding this comment.
Potential inconsistency between frame and context image data
The comments discuss the possibility of inconsistencies if refreshJobCacheIfOutdated is not called in getContextImage. To ensure data consistency, it might be better to call refreshJobCacheIfOutdated in both getFrame and getContextImage.
Consider adding await refreshJobCacheIfOutdated(jobID); at the beginning of the getContextImage function to prevent potential inconsistencies.
export function getContextImage(jobID: number, frame: number): Promise<Record<string, ImageBitmap>> {
return new Promise<Record<string, ImageBitmap>>((resolve, reject) => {
+ await refreshJobCacheIfOutdated(jobID);
if (!(jobID in frameDataCache)) {
reject(new Error(
'Frame data was not initialized for this job. Try first requesting any frame.',
));
}Committable suggestion was skipped due to low confidence.
| async function refreshJobCacheIfOutdated(jobID: number): Promise<void> { | ||
| const cached = frameDataCache[jobID]; | ||
| if (!cached) { | ||
| throw new Error('Frame data cache is abscent'); | ||
| } | ||
|
|
||
| const META_DATA_RELOAD_PERIOD = 1 * 60 * 60 * 1000; // 1 hour | ||
| const isOutdated = (Date.now() - cached.fetchTimestamp) > META_DATA_RELOAD_PERIOD; | ||
|
|
||
| if (isOutdated) { | ||
| // get metadata again if outdated | ||
| const meta = await getFramesMeta('job', jobID, true); | ||
| if (new Date(meta.chunksUpdatedDate) > new Date(cached.meta.chunksUpdatedDate)) { | ||
| // chunks were re-defined. Existing data not relevant anymore | ||
| // currently we only re-write meta, remove all cached frames from provider and clear cached context images | ||
| // other parameters (e.g. chunkSize) are not supposed to be changed | ||
| cached.meta = meta; | ||
| cached.provider.cleanup(Number.MAX_SAFE_INTEGER); | ||
| for (const frame of Object.keys(cached.contextCache)) { | ||
| for (const image of Object.values(cached.contextCache[+frame].data)) { | ||
| // close images to immediate memory release | ||
| image.close(); | ||
| } | ||
| } | ||
| cached.contextCache = {}; | ||
| } | ||
|
|
||
| cached.fetchTimestamp = Date.now(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Typographical error in error message
There is a typo in the error message at line 503.
Apply this diff to correct the typo:
- throw new Error('Frame data cache is abscent');
+ throw new Error('Frame data cache is absent');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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function refreshJobCacheIfOutdated(jobID: number): Promise<void> { | |
| const cached = frameDataCache[jobID]; | |
| if (!cached) { | |
| throw new Error('Frame data cache is abscent'); | |
| } | |
| const META_DATA_RELOAD_PERIOD = 1 * 60 * 60 * 1000; // 1 hour | |
| const isOutdated = (Date.now() - cached.fetchTimestamp) > META_DATA_RELOAD_PERIOD; | |
| if (isOutdated) { | |
| // get metadata again if outdated | |
| const meta = await getFramesMeta('job', jobID, true); | |
| if (new Date(meta.chunksUpdatedDate) > new Date(cached.meta.chunksUpdatedDate)) { | |
| // chunks were re-defined. Existing data not relevant anymore | |
| // currently we only re-write meta, remove all cached frames from provider and clear cached context images | |
| // other parameters (e.g. chunkSize) are not supposed to be changed | |
| cached.meta = meta; | |
| cached.provider.cleanup(Number.MAX_SAFE_INTEGER); | |
| for (const frame of Object.keys(cached.contextCache)) { | |
| for (const image of Object.values(cached.contextCache[+frame].data)) { | |
| // close images to immediate memory release | |
| image.close(); | |
| } | |
| } | |
| cached.contextCache = {}; | |
| } | |
| cached.fetchTimestamp = Date.now(); | |
| } | |
| } | |
| async function refreshJobCacheIfOutdated(jobID: number): Promise<void> { | |
| const cached = frameDataCache[jobID]; | |
| if (!cached) { | |
| throw new Error('Frame data cache is absent'); | |
| } | |
| const META_DATA_RELOAD_PERIOD = 1 * 60 * 60 * 1000; // 1 hour | |
| const isOutdated = (Date.now() - cached.fetchTimestamp) > META_DATA_RELOAD_PERIOD; | |
| if (isOutdated) { | |
| // get metadata again if outdated | |
| const meta = await getFramesMeta('job', jobID, true); | |
| if (new Date(meta.chunksUpdatedDate) > new Date(cached.meta.chunksUpdatedDate)) { | |
| // chunks were re-defined. Existing data not relevant anymore | |
| // currently we only re-write meta, remove all cached frames from provider and clear cached context images | |
| // other parameters (e.g. chunkSize) are not supposed to be changed | |
| cached.meta = meta; | |
| cached.provider.cleanup(Number.MAX_SAFE_INTEGER); | |
| for (const frame of Object.keys(cached.contextCache)) { | |
| for (const image of Object.values(cached.contextCache[+frame].data)) { | |
| // close images to immediate memory release | |
| image.close(); | |
| } | |
| } | |
| cached.contextCache = {}; | |
| } | |
| cached.fetchTimestamp = Date.now(); | |
| } | |
| } |
| if (isOutdated) { | ||
| // get metadata again if outdated | ||
| const meta = await getFramesMeta('job', jobID, true); | ||
| if (new Date(meta.chunksUpdatedDate) > new Date(cached.meta.chunksUpdatedDate)) { |
There was a problem hiding this comment.
Tip
Codebase Verification
Validate comparison of chunksUpdatedDate - Potential improvement needed
The comparison of meta.chunksUpdatedDate and cached.meta.chunksUpdatedDate is implemented correctly using new Date(). However, there's no explicit validation to ensure these are valid date strings. While the type is declared as string, which is appropriate, consider adding a safeguard against potential Invalid Date issues:
- In the
ifstatement at line 512, add a check to ensure both dates are valid before comparison:
if (!isNaN(new Date(meta.chunksUpdatedDate).getTime()) &&
!isNaN(new Date(cached.meta.chunksUpdatedDate).getTime()) &&
new Date(meta.chunksUpdatedDate) > new Date(cached.meta.chunksUpdatedDate)) {
// existing code
}This addition will prevent potential runtime errors if either chunksUpdatedDate is invalid, improving the robustness of the code.
Analysis chain
Validate comparison of chunksUpdatedDate
Ensure that meta.chunksUpdatedDate and cached.meta.chunksUpdatedDate are valid date strings to avoid potential Invalid Date issues during comparison.
Run the following script to check if chunksUpdatedDate is a valid date string:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if chunksUpdatedDate is a valid date in the meta data
# Test: Extract chunksUpdatedDate and attempt to parse it as a date
# Expect: Both dates should be valid
jq '.chunksUpdatedDate' cvat-core/src/frames.ts | xargs -I {} date -d {}
Length of output: 133
Script:
#!/bin/bash
# Search for usage of chunksUpdatedDate in the TypeScript file
ast-grep --lang typescript --pattern 'new Date($chunksUpdatedDate)' cvat-core/src/frames.ts
# Search for any type assertions or validations related to chunksUpdatedDate
rg "chunksUpdatedDate" cvat-core/src/frames.ts -A 5 -B 5
Length of output: 1515
Motivation and context
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
chunks_updated_dateto track when frame chunks were last updated.Bug Fixes
Documentation