-
Notifications
You must be signed in to change notification settings - Fork 102
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
Install scripts, bdist_wheel, x86 docker and instructions #174
Conversation
Hi @passalis, the majority of my comments has been addressed. Concerning 'requires a lot of RAM', I would rephrase the current sentence With respect to the pip packages, Another thing that came to my mind is that it would probably be good to add brief instructions for the user on how to run unittests to verify the successful installation of the toolkit. |
Actually, it works ok. I was testing a previous image. The image currently uploaded on dockerhub seems to work ok. |
@jelledouwe @vniclas thanks for your response!
I will run some tests now and measure memory consumption.
I will add some instructions for running the tests to verify the installation. |
I added a RAM estimate for installation and updated installation instructions for pip wheel, as well as for running the tests. |
I've moved the instructions to build your own docker image to its own section to avoid confusion. |
I added some instructions for connecting the local X server into the docker (e.g., for displaying OpenCV windows). Also, I removed all references to |
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.
For it to work, personally I had to run xhost +local:root
on my host machine. Otherwise I was getting qt.qpa.xcb: could not connect to display unix:1 qt.qpa.plugin: Could not load the Qt platform plugin "xcb" in "/opendr/venv/lib/python3.8/site-packages/cv2/qt/plugins" even though it was found.
when trying to run detr's inference_demo
. But could be just me.
Other than that, the PR looks good to me.
@jelledouwe if your points have been addressed could you approve the PR or we won't be able to merge it if you have pending request changes (and also we need 2 approvals)
You are right @ad-daniel I also had to run this command, but I though it was not needed eventually. So I revised the instructions. |
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.
Thank you!
This PR adds the following:
setup.py
to correctly install OpenDR packageThis PR also add missing
__init__.py
in toolkit and a__version__
variable according to typical python usage.