Skip to content
This repository has been archived by the owner on Oct 31, 2023. It is now read-only.

add the option to use a FORCE_CUDA to force cuda installation on docker #612

Merged
merged 12 commits into from
Mar 31, 2019
Merged

Conversation

obendidi
Copy link
Contributor

cf #167

this seems to be the best workaround

Following discussion [here](#167), this seemed the best solution
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Mar 27, 2019
@miguelvr
Copy link
Contributor

it is better to add FORCE_CUDA as a docker build arg so that it is still possible to build with CPU support only

@miguelvr
Copy link
Contributor

You can keep it as a default set to true.

Plus, the documentation in INSTALL.md needs to be updated accordingly

Ouail and others added 2 commits March 28, 2019 10:42
docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
Copy link
Contributor

@miguelvr miguelvr left a comment

Choose a reason for hiding this comment

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

LGTM

@miguelvr
Copy link
Contributor

@Bendidi can you also modify the instructions in INSTALL.md? Thanks!

@@ -38,7 +38,8 @@ docker run --rm -it \
-v /tmp/.X11-unix:/tmp/.X11-unix \
--device=/dev/video0:/dev/video0 \
--ipc=host maskrcnn-benchmark \
python demo/webcam.py --min-image-size 300
python demo/webcam.py --min-image-size 300 \
Copy link
Contributor

Choose a reason for hiding this comment

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

is this change necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes the default is ../configs/caffe2/e2e_mask_rcnn_R_50_FPN_1x_caffe2.yaml , while when running from docker and from maskrcnn-benchmark folder it should be configs/caffe2/e2e_mask_rcnn_R_50_FPN_1x_caffe2.yaml

Copy link
Contributor

Choose a reason for hiding this comment

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

alright, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that the demo won't work if you run it from maskrcnn-benchmark folder due to the local includes, so this breaks in that case

Copy link
Contributor

Choose a reason for hiding this comment

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

for example, trying to import predictor won't work if done from maskrcnn-benchmark, and it is explicitly mentioned in the readme that you should cd to demo first, so I'd vote for removing this change

Copy link
Contributor Author

@obendidi obendidi Mar 28, 2019

Choose a reason for hiding this comment

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

The current command in the README.md of the demo doesn't work:

docker run --rm -it \
    -e DISPLAY=${DISPLAY} \
    --privileged \
    -v /tmp/.X11-unix:/tmp/.X11-unix \
    --device=/dev/video0:/dev/video0 \
    --ipc=host maskrcnn-benchmark \
    python demo/webcam.py --min-image-size 300

throws the error FileNotFoundError: [Errno 2] No such file or directory: '../configs/caffe2/e2e_mask_rcnn_R_50_FPN_1x_caffe2.yaml'

this change is just to specify the path to the config file in this particular case:

docker run --rm -it \
    -e DISPLAY=${DISPLAY} \
    --privileged \
    -v /tmp/.X11-unix:/tmp/.X11-unix \
    --device=/dev/video0:/dev/video0 \
    --ipc=host maskrcnn-benchmark \
    python demo/webcam.py --min-image-size 300 \
    --config-file configs/caffe2/e2e_mask_rcnn_R_50_FPN_1x_caffe2.yaml

it should not break any imports in the demo folder

Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed that this is for the docker example.

I have never tried using it with docker, but I know for sure that without docker running the webcam demo code from maskrcnn-benchmark folder will not work, you need to be in the demo folder.

But maybe some paths are setup differently in the dockerfile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wierd, works fine for me outside of demo folder (not in docker) using python 3.6 :

python demo/webcam.py --min-image-size 300 --config-file configs/caffe2/e2e_mask_rcnn_R_50_FPN_1x_caffe2.yaml MODEL.DEVICE cpu

@obendidi
Copy link
Contributor Author

@Bendidi can you also modify the instructions in INSTALL.md? Thanks!

There are no modifications to be added to install.md, the installation instructions stays the same. this is just a fix for the installation on docker problem and doesn't impact the process

@miguelvr
Copy link
Contributor

@Bendidi can you also modify the instructions in INSTALL.md? Thanks!

There are no modifications to be added to install.md, the installation instructions stays the same. this is just a fix for the installation on docker problem and doesn't impact the process

there are changes because there is a new build arg in the docker

@obendidi
Copy link
Contributor Author

@miguelvr @fmassa good to merge ?

@miguelvr
Copy link
Contributor

Okay by me, but @fmassa has to merge

Copy link
Contributor

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Thanks!

@fmassa fmassa merged commit 90c226c into facebookresearch:master Mar 31, 2019
Lyears pushed a commit to Lyears/maskrcnn-benchmark that referenced this pull request Jun 28, 2020
…cker (facebookresearch#612)

* add a FORCE_CUDA flag

Following discussion [here](facebookresearch#167), this seemed the best solution

* Update Dockerfile

* Update setup.py

* add FORCE_CUDA as an ARG

* 	modified:   docker/Dockerfile
	modified:   setup.py

* small fix to readme of demo

* remove test print

* keep ARG_CUDA

* remove env value and use the one from ARG

* keep same formatting as source

* change proposed by @miguelvr

* Update INSTALL.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants