Skip to content
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

Merged
merged 146 commits into from
Dec 23, 2021

Conversation

passalis
Copy link
Collaborator

@passalis passalis commented Dec 14, 2021

This PR adds the following:

  • Scripts to install OpenDR toolkit in clean Ubuntu 20.04 systems (even when running from a minimal image, e.g,. docker ones)
  • setup.py to correctly install OpenDR package
  • Corrected scripts to activate OpenDR venv environment
  • Scripts to create bdist wheels for cpu only usage
  • Dockerfile for assembling cpu-only OpenDR inference
  • Readme listing different installation options
  • Update wiki to reflect the changes made in this PR

This PR also add missing __init__.py in toolkit and a __version__ variable according to typical python usage.

@passalis passalis marked this pull request as draft December 14, 2021 20:38
@passalis passalis added the test sources Run style checks label Dec 14, 2021
@passalis passalis changed the title Install scripts and instructions Install scripts, bdist_wheel, x86 docker and instructions Dec 14, 2021
@vniclas
Copy link
Collaborator

vniclas commented Dec 22, 2021

Hi @passalis, the majority of my comments has been addressed.

Concerning 'requires a lot of RAM', I would rephrase the current sentence make sure that you have enough RAM available with a guess of how much will be actually required.

With respect to the pip packages, detectron2 does not actually trigger overwriting the installed versions, i.e., as long as detectron2 still works as expected, this probably does not matter.

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.

@passalis
Copy link
Collaborator Author

I think bcolz was not built correctly in gpu docker image. I am checking this right now.

Actually, it works ok. I was testing a previous image. The image currently uploaded on dockerhub seems to work ok.

@passalis
Copy link
Collaborator Author

@jelledouwe @vniclas thanks for your response!

Concerning 'requires a lot of RAM', I would rephrase the current sentence make sure that you have enough RAM available with a guess of how much will be actually required.

I will run some tests now and measure memory consumption.

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.

I will add some instructions for running the tests to verify the installation.

@passalis
Copy link
Collaborator Author

I added a RAM estimate for installation and updated installation instructions for pip wheel, as well as for running the tests.

@ad-daniel
Copy link
Collaborator

I've moved the instructions to build your own docker image to its own section to avoid confusion.
Also, before merging this PR we need to remember to change all mentions of the install_scripts branch

@passalis
Copy link
Collaborator Author

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 install_scripts so some commands/instructions are now broken. As soon as we merge this PR, I will test everything again to make sure that they work as intended.

ad-daniel
ad-daniel previously approved these changes Dec 23, 2021
Copy link
Collaborator

@ad-daniel ad-daniel left a 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)

@passalis
Copy link
Collaborator Author

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.

@passalis passalis requested a review from jelledouwe December 23, 2021 08:02
Copy link
Collaborator

@ad-daniel ad-daniel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@passalis passalis merged commit 236fd3a into master Dec 23, 2021
@passalis passalis deleted the install_scripts branch December 23, 2021 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test sources Run style checks test tools Test the toolkit methods
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants