-
Notifications
You must be signed in to change notification settings - Fork 74
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
#7944 add dockerfile and installation steps #7949
#7944 add dockerfile and installation steps #7949
Conversation
9317ebf
to
1c91299
Compare
b31d455
to
58643f2
Compare
scripts/docker/docker_bringup.sh
Outdated
mkdir -p /home/${USER}/workspace | ||
HOST_UID=${UID} HOST_GID=${GID} docker compose up -d | ||
|
||
docker exec -it tt-metal bash |
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.
does the -v
parameter work here? If so, we should give user ability to mount their own folder(s)
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, the -v will not work here but users do have the ability to mount their own folders in the docker-compose.yaml file underneath the 'volumes' section
The syntax of mounting a new volume using docker compose should be the same so hopefully there will not be any difficult learning curve here.
The flow should be as such
user edits docker-compose.yaml - volumes section
user runs ./docker_bringup.sh
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.
Will add small comment in the docker-compose file to indicate to users to mount files here.
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.
update removed docker-compose and all references to it
scripts/docker/docker-compose.yaml
Outdated
- /etc/passwd:/etc/passwd:ro | ||
- /etc/shadow:/etc/shadow:ro | ||
# Allow for persist file storage | ||
- /home/${USER}/workspace:/home/ubuntu:rw |
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.
when user enters docker, their username changes to ubuntu?
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, but they should have the same permissions as the user itself
dockerfile/tt-metal-x86.Dockerfile
Outdated
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.
should this be renamed to Ubuntu-20.04-x86.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.
yes, good point, will rename.
tt-metal-x86 is probably redundant here
@adifrancescoTT |
I'll be honest, I find this usage of docker-compose to be... odd. It's usually used to run a service permanently. Sure, that's how we use Docker internally, to set up a container that you ssh into to do work, but we don't have to use it that way. I have kept a similar Dockerfile locally, and I just launch the build and test from within it, without "moving into" the docker environment. So, I have a launch script that runs the build, like the below. Note: This isn't for sharing directly, since it has paths/dependencies on my paths, etc. #!/bin/bash -x
set -e
export CONTAINER=2404:latest
[ -d /work/.pipcache ] || mkdir /work/.pipcache
function run_docker {
sudo docker run \
--rm -t -i \
-v /work:/work \
-v /home:/home \
-v /dev/hugepages-1G:/dev/hugepages-1G \
-w /work/tt-metal \
-e TT_METAL_HOME=/work/tt-metal \
-e TT_METAL_ENV=dev \
-e ARCH_NAME=grayskull \
-e PYTHONPATH=/work/tt-metal \
-e XDG_CACHE_HOME=/work/.pipcache \
-e DEVICE_CXX=/usr/bin/clang++ \
-e SILENT=0 \
--net host \
--device /dev/tenstorrent/0 \
--privileged \
${CONTAINER} \
"$@"
}
run_docker script -c "make build -k -j$(nproc)" build.out.$(date +%Y%m%d%H%M%S)
run_docker script -c "make tests -k -j$(nproc)" tests.out.$(date +%Y%m%d%H%M%S)
#run_docker script -c ". ./build/python_env/bin/activate ; ./tests/scripts/run_tests.sh --tt-arch grayskull --pipeline-type post_commit" pytest.out.$(date +%Y%m%d%H%M%S)
|
FWIW, the Dockerfile I've been using is similar but maybe a bit less verbose. This is the Ubuntu 24.04 version: FROM ubuntu:24.04
# Local setup to use a proxy server for all package installs for speedup
#COPY 00aptproxy /etc/apt/apt.conf.d
RUN apt-get update
RUN apt-get install -y --no-install-recommends apt-utils dialog
RUN DEBIAN_FRONTEND=noninteractive apt-get install -y libboost-all-dev sudo zip unzip
RUN DEBIAN_FRONTEND=noninteractive apt-get install -y tmux vim git
RUN apt-get install -y pciutils kmod python3-pip
RUN apt-get install -y python3 python3-dev python3-venv
RUN apt-get install -y curl jq
RUN apt-get install -y software-properties-common build-essential python3-venv libgoogle-glog-dev libyaml-cpp-dev libboost-all-dev libsndfile1 libhwloc-dev
RUN apt-get install -y clang git git-lfs cmake pandoc libtbb-dev libcapstone-dev pkg-config
RUN apt-get install -y python-is-python3
RUN apt-get install -y clang
# Ubuntu 24.04 has a new enough rustc/cargo, so just go with that instead of rustup for now
RUN apt-get install -y rustc cargo
# Doxygen and gtest install
RUN curl -L "https://www.doxygen.nl/files/doxygen-1.9.6.linux.bin.tar.gz" -o doxygen-1.9.6.linux.bin.tar.gz
RUN tar -xvf doxygen-1.9.6.linux.bin.tar.gz
RUN cd doxygen-1.9.6 && sudo make install
RUN curl -L "https://github.com/google/googletest/archive/refs/tags/v1.13.0.tar.gz" -o v1.13.0.tar.gz
RUN tar -xvf v1.13.0.tar.gz
RUN cd googletest-1.13.0 && cmake -DCMAKE_INSTALL_PREFIX=/usr -DBUILD_SHARED_LIBS=ON . && make && sudo make install
# 24.04 comes with a pre-populated ubuntu account, which conflicts with my own default UID,
# so delete it and create one with my own username. This is obviously non-portable.
RUN deluser ubuntu
RUN useradd -m -s /bin/bash -u 1000 olof
USER olof |
INSTALLING.md
Outdated
[Docker](https://docs.docker.com/engine/install/ubuntu/#install-using-the-repository) version 26.1.0 | ||
[Docker Compose](https://docs.docker.com/compose/install/linux/#install-using-the-repository) version v2.26.1 | ||
|
||
### Step 4. Install Docker, Docker Compose, and Build Docker Images |
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.
Review this and following steps numbering
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 want to make sure that keeping both installation paths in the same file (vs 2 Develop in Docker and Develop on host files) is the easiest option. If we don't need to go thorugh tt-smi/tt-flash/tt-kmd then I'd recommend separating the two.
If we end up keeping them in the same file, there's a minor typo in the numbering of the steps from 4 on.
INSTALLING.md
Outdated
@@ -56,23 +56,62 @@ sudo -E python3 setup_hugepages.py enable && sudo -E python3 setup_hugepages.py | |||
``` | |||
--- | |||
|
|||
### Step 4. Build from source and start using TT-NN and TT-Metalium! | |||
### Step 4. Clone tt-metal repo |
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.
Are the steps before this one, and particularly step 1, still required for the Docker path? I see Install "tt-smi/tt-flash/tt-topology" in the Dockerfile so want to make sure
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.
So the installation Hugepages (Step 3) and tt-kmd drivers must be installed on the host side as this can't be done in docker. The firmware can be flashed using docker too.
In essence, half of Step 1 and Step 3 are required, and Step 2 is not required if developing in docker.
I think a split into two files is probably a better choice here
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.
Thanks for clarifying @ttmchiou and agree with that. We could link the Docker installation steps right at the beginning of this file by adding something similar to what I share here in Italic (please feel free to rephrase):
"These instructions will guide you through the installation of Tenstorrent system tools and drivers, followed by the installation of TT-Metalium and TT-NN.
The following steps will help you configure a native host development environment. If you prefer to work in a Docker container, please follow this guide: <link_to_docker_steps>."
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.
@adifrancescoTT Do you think its better to have it split into two files or just two independent sections but still living in INSTALLING.md?
Option 1: INSTALLING.md + DOCKER_INSTALLING.md
or
Option 2: INSTALLING.md
but it would look something like
Intro
for native developemnt, go here
for docker development, go here
Native Development
Instructions
Docker Development
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.
Not a super strong preference, but I think I'm leaning towards what you're suggesting (option 2) so go with it if you're also aligned. My main ask is that each section is fully "self-contained", meaning a user can follow it from start to end and achieve their goal without referencing the other one. Thanks!
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.
@adifrancescoTT
Updated, let me know your thoughts
@olofj I don't have any strong opinions either way on this so I'm happy to remove docker compose and use your method if you think using it this way is too strange. |
82510be
to
81c1105
Compare
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.
Unless I'm misreading, I think the Native Development section needs some changes, but the overall structure works.
INSTALLING.md
Outdated
@@ -61,7 +67,7 @@ sudo -E python3 setup_hugepages.py enable && sudo -E python3 setup_hugepages.py | |||
1. Install git and git-lfs | |||
|
|||
```sh | |||
sudo apt install git git-lfs | |||
sudo apt install git git-lfs python3-pip | |||
``` | |||
|
|||
3. Clone the repo. |
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.
@ttmchiou I believe these steps were different, they also look out of order now (1., 3., 2.). Clone the repo should have instructions on how to clone. Actually looking at Step 3 of Docker, that's how this should look like (with environment variables too). Is it possible there was some typo reorganizing the code?
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.
@adifrancescoTT
Whoops, i accidently added python3-pip as a requirement in the Native Host Installation section.
Other than that though, I haven't made any changes to that section so I'm not sure what you mean by instructions on how to clone?
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.
moved changes to INSTALLING.md to another ticket.
#8550
a4dd421
to
55bf5ad
Compare
35febb8
to
4e7c915
Compare
4e7c915
to
9655bea
Compare
Split this PR into smaller PRs |
9655bea
to
0beb633
Compare
0beb633
to
1b95a4e
Compare
1b95a4e
to
8c2122e
Compare
CODEOWNERS
Outdated
@@ -162,3 +162,6 @@ tests/python_api_testing/conv/ @tt-nshanker | |||
tests/python_api_testing/unit_testing/fallback_ops @tt-aho | |||
scripts/profiler/ @mo-tenstorrent | |||
scripts/install @tapspatel | |||
scripts/docker @ttmchiou @TT-billteng @tt-rkim | |||
|
|||
dockerfile @ttmchiou @TT-billteng @tt-rkim |
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.
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.
lol
scripts/docker/requirements.txt
Outdated
python3.8-venv=3.8.10-0ubuntu1~20.04.9 | ||
|
||
cargo | ||
ninja-build |
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.
new line
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.
Also damn, I'm wondering if we can capture these requirements in one nice file somewhere rather than copy pasta in
- miw playbooks
- dev deps JSONs in
.github/actions
- README
probably some other places
@@ -0,0 +1,5 @@ | |||
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.
Damn sudo
is a requirement?
Also, can we add vim
?
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.
added vim to requirements
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 I know why vim was in my docker container LOL!
scripts/docker/requirements_dev.txt
Outdated
sudo | ||
nano | ||
acl=2.2.53-6 | ||
jq=1.6-1ubuntu0.20.04.1 |
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 need to pin acl and jq
COPY /scripts/docker/requirements_dev.txt /opt/tt_metal_infra/scripts/docker/requirements_dev.txt | ||
RUN apt-get -y update \ | ||
&& xargs -a /opt/tt_metal_infra/scripts/docker/requirements_dev.txt apt-get install -y --no-install-recommends \ | ||
&& rm -rf /var/lib/apt/lists/* |
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.
Why delete this cache? IS it for something later?
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.
keeps the docker layer smaller.
COPY /scripts /opt/tt_metal_infra/scripts | ||
COPY build_metal.sh /scripts/build_metal.sh | ||
|
||
# ENV TT_METAL_INFRA_DIR=/opt/tt_metal_infra |
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 we delete this extra commented stuff?
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.
Will be for future work in including Virtual Env in Docker. This will be integrated but just disabled for future reference
removing pinned version of jq and acl posix lines for CODEOWNERS adding vim to requirements_dev.txt
4210ecb
to
d87cd71
Compare
This PR specifically adds the following
I have separated previous implementation details into the following separate PRs
Revised INSTALLING.md to incorporate using docker for install workflows. Update INSTALLING.md for docker instructions #8550
I had to re-order some of the steps but hopefully instructions should be clear enough.
I believe you can close your eyes and just copy all the code blocks and it should still work
#7944: add convenience script that installs docker deps and compose #8566
Added scripts/docker: This folder holds relevant shell scripts to build/install docker and necessary dependencies. Also includes a shell script to bring up and automatically bring user into the docker container to begin development
#7944: add convenience script that installs docker deps and compose #8566
#7944: Add docker convenience installation script for devs #8565