Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Per segment chunks #8272
Per segment chunks #8272
Changes from 80 commits
b32a9eb
cb4ff93
d49233c
146a896
52d1bac
0c53436
c166123
630c97e
8d710e7
654a827
12e5f2a
a79a681
d5118a2
1b429cf
d512312
167ee12
88a9cb2
30bf8fd
ee3c905
dc03220
4bb8a74
f7d2c4c
34d9ca0
8c97967
a0fd0ba
c0480c9
5caf283
efbe3a0
fb1284d
8edcfc5
51a7f83
5a2a746
65e4174
61f1735
34f972f
2bb2b17
3788917
f695ae1
14a9033
e8bebe9
bbef52f
3d5bb52
0e9c5c8
351bdc8
a71852c
d90ca0d
03e749a
c0822a0
56d413f
48f4794
5c0cc1a
5abd891
749b970
9105cd3
8dafcbe
4cbf82f
88c34a3
b0fd006
2eac04a
640518c
3a246b3
a0704f4
cf026ef
f73cef3
258c800
b3ae317
5e89ef4
c5edcda
7f5c722
daf4035
8c1b82c
f172865
aacceee
c8dbb7c
6b9a3e9
f5661e4
754757f
0c001a5
a9390eb
d2b1385
c9a5e31
621afa7
e40ffd1
08c9f01
92a19f4
441d0e7
0963f94
0d78e63
f53948d
e69f2b7
1a9a813
e4db8ad
b1c54f9
5312b00
3c117fe
98eff81
316ec78
ebed825
92f6083
2b6e987
f67a1a2
d72fe85
d5bfb88
c5a1197
a5cf3b7
069f48c
cfdde3f
843b957
feb92cd
2424f2b
fe60bdf
6ddb6bf
4fa7b97
32f1be2
0e95b40
21135b7
b311f1e
79bb1f7
55a8424
add5ae6
643d998
df90b33
6ccb7db
1fb68bc
92d0c7a
67c1650
3cdc4dc
716042e
19279c7
bc5ed39
08ddd28
1d969bd
e2cba8c
d135475
a1638c9
fc89c01
c6e65f6
118828a
3eea60d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
There were only permanent chunks on the local filesystem, what did you want to reflect in the change?
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.
Regular users (even those who may install self-hosted solution) do not know such implementation details.
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.
I just want to clarify that this is exactly about permanent chunks, explicitly
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.
Ok, but it's just what is passed in the task data endpoint API:
storage_method = 'file_system' | 'cache'
.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.
Updated the message
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.
I would suggest to formulate somehow more clear for end user (not sure actually how exactly).
But this record does not say me (as a CVAT user anything). This is probably only important for REST API users
Maybe: Numeration of data chunks in any job always starts with 0
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.
By the way this change is breaking. Thus it means that we have to up major version of REST API.
Hovewer I am not sure we want do it now. Probably worthy of internal discussion
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.
SemVer is not used in the server REST API now.
Ok, do you want me to add some tag to clarify that this is for API users or do you want it to be removed from the changelog? REST API users are users as well.
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.
Why did you decide so?
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.
We never established SemVer for the server component. There were discussions a couple of years ago, we decided to postpone it. Currently, we can have any changes in the server API in any release, the version is the same as in the release itself.
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.
As I mentioned in the beginning:
As I, from another hand, see strong reasons do not update version right 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.
Maybe instead of modifying existing
chunk_number
parameter, we have to add one more and deprecate previous behaviourThere 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.
Yes, it's certainly doable.
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.
Added the
index
parameter.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.
Implemented + refactored the UI a little bit. There are 2 potential improvements to be done: