-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[Dependend] Reset chunks from cache if oudated #8449
[Dependend] Reset chunks from cache if oudated #8449
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 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_date
is appropriate and consistentThe property
chunks_updated_date: string;
is properly added to theSerializedFramesMetaData
interface, following the existing naming conventions and type usage for date properties in this file.cvat-core/src/frames.ts (7)
19-19
: Initialization offetchTimestamp
property is appropriateThe addition of the
fetchTimestamp
property toframeDataCache
will help track when the metadata was last fetched. This is essential for ensuring that outdated cache entries are refreshed timely.
59-59
: Addition ofchunksUpdatedDate
toFramesMetaData
Including
chunksUpdatedDate
in theFramesMetaData
class allows the system to detect changes in chunk definitions effectively.
74-74
: Settingchunks_updated_date
to undefined in initial dataEnsuring
chunks_updated_date
is initialized is important for accurate comparisons later in the code.
139-141
: Definition of getter forchunksUpdatedDate
The getter method for
chunksUpdatedDate
correctly retrieves the value fromdata.chunks_updated_date
.
507-507
: EnsurefetchTimestamp
is always initializedIn the calculation of
isOutdated
, ifcached.fetchTimestamp
isundefined
, it could lead to unexpected results.Please confirm that
fetchTimestamp
is always initialized beforerefreshJobCacheIfOutdated
is called. If there is a possibility of it beingundefined
, consider initializing it with a default value.
653-653
: Initialization offetchTimestamp
property
fetchTimestamp
is correctly initialized with the current timestamp when creating the cache for a job.
709-709
: Type annotation forid
parameter ingetDeletedFrames
Specifying the type of
id
asnumber
enhances type safety and clarity.cvat-core/src/server-proxy.ts (2)
1484-1488
: Ensure Type Consistency withSerializedFramesMetaData
The
getMeta
function now returns an object with an additional propertychunks_updated_date
. This property may not be defined in theSerializedFramesMetaData
interface, which could lead to type inconsistencies or TypeScript compilation errors.Consider updating the
SerializedFramesMetaData
interface to include thechunks_updated_date
property, or adjust the return type to accurately reflect the returned object.You can run the following script to verify if
chunks_updated_date
is included in theSerializedFramesMetaData
type definition:Verification successful
chunks_updated_date
is Already Defined inSerializedFramesMetaData
The verification process has confirmed that the
chunks_updated_date
property is already included in theSerializedFramesMetaData
interface definition. This property is defined as a string in the interface, which is consistent with the implementation in thegetMeta
function.
- 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_date
is 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 withSerializedFramesMetaData
The
saveMeta
function now returns an object with an additional propertychunks_updated_date
. Similar to the previous comment, this property may not be declared in theSerializedFramesMetaData
interface, potentially causing type inconsistencies or TypeScript compilation errors.Please update the
SerializedFramesMetaData
interface to includechunks_updated_date
, or adjust the return type accordingly.You can run the following script to verify if
chunks_updated_date
is included in theSerializedFramesMetaData
type definition:
cvat-core/src/frames.ts
Outdated
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 refreshed
Also, 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 |
cvat-core/src/frames.ts
Outdated
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
cvat-core/src/frames.ts
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
if
statement 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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## zm/validation-management-core #8449 +/- ##
=================================================================
- Coverage 74.16% 74.15% -0.02%
=================================================================
Files 396 396
Lines 43046 43065 +19
Branches 3897 3900 +3
=================================================================
+ Hits 31927 31936 +9
- Misses 11119 11129 +10
|
c541fbb
into
zm/validation-management-core
Quality Gate passedIssues Measures |
Motivation and context
Server part must be implemented before merging the pull request: #8348
How has this been tested?
Checklist
develop
branch(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_date
to track when frame chunks were last updated.Bug Fixes
Documentation