-
Notifications
You must be signed in to change notification settings - Fork 5
Docker mac #8
base: master
Are you sure you want to change the base?
Docker mac #8
Conversation
LABEL Author="Lara CODECA ([email protected])" | ||
LABEL Description="Dockerised SUMO-devel with tensorflow-gpu environment." | ||
|
||
ARG USER_ID |
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 are not using
ARG USER_ID
ARG GROUP_ID
and you hardcoded them in
RUN groupadd --gid 222 alice && \
useradd -m -s /bin/bash -u 502 -g 222 alice && \
echo "alice:alice" | chpasswd && adduser alice sudo
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.
Updated in commit ac6dd1c
docs/howto-mac.md
Outdated
|
||
## Step 2: Run a container of the built image | ||
|
||
`sudo bash docker-cmd-mac.sh --image-name=tf-gpu-sumo-2020-08-03 --run --no-gpu` |
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.
Can you use tf-gpu-sumo-{today_date}
instead of a specific date?
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.
Updated in commit 0d33ba4
docker-cmd-mac.sh
Outdated
else | ||
DETACH="" | ||
fi | ||
CURR_UID=$(id -u) |
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.
Here the variables
CURR_UID=$(id -u)
CURR_GID=$(id -g)
are still set and pass to docker, even if they are hardcoded in the Dockerfile.
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.
now can also be set through command line 3700450
docker-cmd-mac.sh
Outdated
IMAGE_FOLDER="docker-image-mac" | ||
GPU=true | ||
GPU_OPT="--gpus all" | ||
OPTIRUN=false |
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.
optirun does not exist for Mac OSX
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.
Updated in the commit 97d2053
docs/howto-mac.md
Outdated
|
||
## Step 2: Run a container of the built image | ||
|
||
`sudo bash docker-cmd-mac.sh --image-name=tf-gpu-sumo-2020-08-03 --run --no-gpu` |
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.
Running docker using sudo it's not a good practice.
Given that without it does not work, and it's a problem of user privilege between host and container, it needs to be highlighted as a security issue, at least in the HOWTO.
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.
Updated in the commit ad4dbca
|
||
# Download & install RLLIB+SUMO Utils | ||
WORKDIR /home/alice/libraries | ||
RUN git clone --depth 1 https://github.com/lcodeca/rllibsumoutils.git rllibsumoutils |
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.
Apologies for this change that I applied today.
Given that the latest devel version may be unstable, I changed it from main branch to v1_6_0
See 42a5dec
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.
no worries, synchronised now f0d44a0
@@ -0,0 +1,65 @@ | |||
# RLLIB SUMO Docker environment for MacOS |
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.
Would you mind adding yourself as the author of this document?
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.
thx for the advice, added now 1301a20
docs/howto-mac.md
Outdated
|
||
`sudo bash docker-cmd-mac.sh --image-name={your_image_name} --run --no-gpu` | ||
|
||
> :warning: **Using SUDO for docker is not a good practice**: Due to a problem of user privilege between host and container, `sudo` has to be used here to make it work. We will address this security issue soon. |
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.
We will address this security issue soon.
Are you sure it's addressable in the first place?
Otherwise, we can keep warning and remove the last statement, or we can say "We will address this security issue, eventually.".
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 revised the last sentence accordingly c5d0277
COMMAND="" | ||
EXP="" | ||
DETACH=false | ||
|
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 had to add a new parameter, due to the warning issued by the deep RL infrastructure.
See fd5ea7a
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.
synchronised now 7d3d349
No description provided.