-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Manifest optimization #3712
Manifest optimization #3712
Conversation
ad6331b
to
68c7176
Compare
@Marishka17 , it will be great to have a line in CHANGELOG. Probably you can estimate some speed up which we should get in some scenarios. It will be helpful as well. |
@Marishka17 , need to add tqdm module into requirements
|
utils/dataset_manifest/core.py
Outdated
file.write(f'{json_item}\n') | ||
|
||
def _write_core_part(self, file): | ||
for item in tqdm(self._reader, |
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.
It is better don't use tqdm inside a library. It is a good solution for create.py. What do you think?
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 think that need to use tqdm
where an iteration directly takes place. It seems to me not a very good solution to raise an iteration to a higher level.
If we will use tqdm
in create.py, we should change manifest.create()
to for _ in manifest.create(): pass
in all places where manifest.create()
is used, or expand existing API. What do you think about it?
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.
Does it spam CVAT server logs with unnecessary messages?
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.
In general I don't advice a solution. I just want to say that the general rule is to use tqdm inside CLI not in a library.
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.
You can pass an optional object into the create
method. Thus CLI will pass tqdm like object. If the code is imported, the object will be None.
Resolve #2993
Motivation and context
How has this been tested?
Manually, old tests work as expected
Checklist
develop
branch- [ ] I have updated the documentation accordingly- [ ] I have added tests to cover my changes- [ ] I have increased versions of npm packages if it is necessary (cvat-canvas,cvat-core, cvat-data and cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
- [ ] I have updated the license header for each file (see an example below)