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.

Sign up for GitHub

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

Jump to bottom

Publish Release Images #15013

Merged
merged 8 commits into from
Nov 26, 2024
Merged

Publish Release Images #15013

merged 8 commits into from
Nov 26, 2024

Conversation

dimitri-tenstorrent
Copy link
Contributor

@dimitri-tenstorrent dimitri-tenstorrent commented Nov 13, 2024 •
edited
Loading

Ticket

#12496
#12495 (describes agreed upon naming scheme)

Problem description

Allow users to get started with TT-Metal stack within two command lines.

docker pull ghcr.io/tenstorrent/tt-metal/tt-metalium/ubuntu-20.04-amd64-release/wormhole_b0:latest
docker run --rm -it -t ghcr.io/tenstorrent/tt-metal/tt-metalium/ubuntu-20.04-amd64-release/wormhole_b0:latest python3 -c "import ttnn"

What's changed

Add a job for the package release to also build/push a release image.

TODO:

  • Document the expected naming scheme for all of the images
  • Add documentation for the users to get started with the Docker images
  • Set the latest tag only when deploying the major version.
  • Add smoke test for the docker image
  • Create a test version of the release image
  • Clean up a lot of test folders from our docker registry

Checklist

  • Post commit CI passes
  • Blackhole Post commit (if applicable)
  • Model regression CI testing passes (if applicable)
  • Device performance regression CI testing passes (if applicable)
  • New/Existing tests provide coverage for changes

@dimitri-tenstorrent dimitri-tenstorrent force-pushed the dimitri/12496_release_images branch 2 times, most recently from df099cc to b38aaf8 Compare November 21, 2024 20:07
@dimitri-tenstorrent dimitri-tenstorrent marked this pull request as ready for review November 22, 2024 16:29
@blozano-tt
Copy link
Contributor

Is it normal to distribute docker images through github container registry?

I thought dockerhub was the norm.

blozano-tt
blozano-tt approved these changes Nov 22, 2024
View reviewed changes
Copy link
Contributor

@blozano-tt blozano-tt left a comment

Choose a reason for hiding this comment

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

LGTM!

Only complaint is that this is not really a release docker image, as it has build dependencies. However, I see you've documented that and filed an issue to fix.

@Dimitri-Gnidash
Copy link

Is it normal to distribute docker images through github container registry?

I thought dockerhub was the norm.

I do think it is the norm and definitely more appropriate for the current state of these images. We can always change the destination registry at a later time.

@dimitri-tenstorrent dimitri-tenstorrent mentioned this pull request Nov 22, 2024
Merged
5 tasks
dimitri-tenstorrent added a commit that referenced this pull request Nov 22, 2024
@dimitri-tenstorrent
#12496: Add workflow wrapper for pre-release testing of PR#15013 for…
c1f3bec 
… Release Images (#15371)

### Ticket
#12496
#12495 (describes agreed
upon naming scheme)

### Problem description

This is an empty workflow that allows for additional testing before the
main set of changes is merged from
#15013

### What's changed

Added a new NO-OP workflow

### Checklist
- [x] Post commit CI passes
- [ ] Blackhole Post commit (if applicable)
- [ ] Model regression CI testing passes (if applicable)
- [ ] Device performance regression CI testing passes (if applicable)
- [ ] New/Existing tests provide coverage for changes
dimitri-tenstorrent
dimitri-tenstorrent commented Nov 26, 2024
View reviewed changes
.github/actions/docker-run/action.yml
@@ -86,7 +86,7 @@ runs:
${{ inputs.device }}
-w ${{ github.workspace }}
run: |
if [ ${{ inputs.install_wheel }} ]; then
if [ "${{ inputs.install_wheel }}" == "true" ]; then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix the issue where the wheel was always installed.

dimitri-tenstorrent
dimitri-tenstorrent commented Nov 26, 2024
View reviewed changes
.github/actions/generate-docker-tag/action.yml
@@ -24,8 +24,8 @@ runs:
- name: Determine Full Docker Image Tag
shell: bash
run: |
echo "TT_METAL_DOCKER_IMAGE_TAG=ghcr.io/${{ github.repository }}/tt-metalium/${{ inputs.image }}:${{ env.IMAGE_TAG }}" >> $GITHUB_ENV
echo "TT_METAL_REF_IMAGE_TAG=ghcr.io/${{ github.repository }}/tt-metalium/${{ inputs.image }}:latest" >> $GITHUB_ENV
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does not allow us to use images that follow the convention outlined in:
#12495
i.e.
tt-metalium-{OS}-{ARCH}-release/grayskull

@dimitri-tenstorrent
#11973: Create a workflow for creating release images
5e1486b 
 #12496: Add the correct version info
 #12496: Add the documentation instructions
 #12496: Update to latest upon major version releases
 #12495: Adjust the release naming scheme to match what was discussed in the ticket
 #12496: Update the installation instructions to the correct naming scheme.
@dimitri-tenstorrent
#12496: Add the smoke test after the image creation
f4be696
@dimitri-tenstorrent
#12496: Fix the reference to the workflow
d6d7ea1 
 #12496: Build wheels before running the docker release workflow
@dimitri-tenstorrent
#12496: Do not require a particular prefix in generate-docker-tag
37aba34
@dimitri-tenstorrent
#12496: Rework the workflow into three parts to leverage different b…
3224e38 
…uilders

 #12496: Respect not installing the wheel with the image
@dimitri-tenstorrent
#12496: Respect not installing the wheel with the image
cfea754 
 #12496: Clean up the echo statements
@dimitri-tenstorrent
Copy link
Contributor Author

Should we update the latest tag only when the major release happens?

  - name: Tag latest if this is a major version release
    if: ${{ inputs.is_major_version }}

@dimitri-tenstorrent dimitri-tenstorrent merged commit 845ddf8 into main Nov 26, 2024
10 of 11 checks passed
@dimitri-tenstorrent dimitri-tenstorrent deleted the dimitri/12496_release_images branch November 26, 2024 18:29
tt-rkim
tt-rkim reviewed Nov 26, 2024
View reviewed changes
.github/workflows/publish-release-image.yaml
arch: wormhole_b0,
runs-on: ["cloud-virtual-machine", "N150", "in-service"],
cmd: pytest tests/end_to_end_tests,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not N300?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I picked the machine that seemed to have a lesser demand.

tt-rkim added a commit that referenced this pull request Nov 26, 2024
@tt-rkim
#0: Revert "Publish Release Images (#15013)" because docker image nam…
364e3fd 
…e is not

    being passed properly for release image

This reverts commit 845ddf8.
@dimitri-tenstorrent dimitri-tenstorrent restored the dimitri/12496_release_images branch November 26, 2024 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tt-rkim tt-rkim tt-rkim approved these changes

@blozano-tt blozano-tt blozano-tt approved these changes

@ttmchiou ttmchiou ttmchiou approved these changes

@TT-billteng TT-billteng Awaiting requested review from TT-billteng TT-billteng is a code owner

@afuller-TT afuller-TT Awaiting requested review from afuller-TT afuller-TT is a code owner

Assignees

@tt-rkim tt-rkim

@ttmchiou ttmchiou

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dimitri-tenstorrent @blozano-tt @Dimitri-Gnidash @tt-rkim @ttmchiou