-
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
Fix create manifest command #5172
Conversation
@@ -60,8 +60,7 @@ optional arguments: | |||
### Alternative way to use with cvat/server | |||
|
|||
```bash | |||
docker run -it --entrypoint python3 -v /path/to/host/data/:/path/inside/container/:rw cvat/server | |||
utils/dataset_manifest/create.py --output-dir /path/to/manifest/directory/ /path/to/data/ | |||
docker run -it -u root --entrypoint bash -v /path/to/host/data/:/path/inside/container/:rw cvat/server -c "pip3 install -r utils/dataset_manifest/requirements.txt && python3 utils/dataset_manifest/create.py --output-dir /path/to/manifest/directory/ /path/to/data/" |
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.
Do I understand correctly that this solution offers to download and install packages every time?
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.
Yes, and also I know that it is not a good idea to run pip with sudo/docker with root. But it is the simplest way for the user. (I mean that from my point of view it is not comfortable for the user to create some docker file that inherits from our server image and then run the command. Another way is adding tqdm module to server requirements but it doesn't suit us. And maybe the last variant is to install in code). So, how do you suggest improving this one?
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.
My ideas are #5096 or, at least, modifying the server container to have this environment installed, if this module is supposed to be used this way. Otherwise, it's just a hack, and its hard to be called usable. Just imagine a user that tries to find out the correct way to invoke the command - so is is expected to be executed multiple times in a row - and each time he needs to wait for downloading and installation. The user also needs internet connection to run this command.
Another way is adding tqdm module to server requirements but it doesn't suit us. And maybe the last variant is to install in code).
I can't agree with that. The server directly uses this package in the source code, it is essential to install its dependencies. Probably, having a separate inherited image, or using the server one are the preferred ways, because server uses a custom version of ffmpeg, and it can heavily affect the results.
Motivation and context
How has this been tested?
Checklist
develop
branch- [ ] I have added tests to cover my changes- [ ] I have linked related issues (read github docs)- [ ] 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.