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

#12775: Cleanup docker run action #12777

Merged
merged 1 commit into from
Sep 24, 2024
Merged

#12775: Cleanup docker run action #12777

merged 1 commit into from
Sep 24, 2024

Conversation

dimitri-tenstorrent
Copy link
Contributor

@dimitri-tenstorrent dimitri-tenstorrent commented Sep 17, 2024 •
edited
Loading

Ticket

#12775

Problem description

During the review several issues were highlighted of how to make the Docker Run Action be a cleaner interface.

What's changed

  • Remove unnecessary arch parameter of the docker run action
  • Add a parameter to installing the wheel
  • Remove LD_LIBRARY env variable since it is not used in the wheel environments
  • Remove Github username since it is accessible inside of the custom Action

Additional features we might want in this PR

  • Parametrize the image name inside of Docker Run Action
  • Run the docker system prune as the first step for self-healing

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 commented Sep 17, 2024
View reviewed changes
.github/workflows/models-post-commit.yaml
@@ -64,12 +62,9 @@ jobs:
timeout-minutes: ${{ inputs.timeout }}
uses: ./.github/actions/docker-run
with:
docker_username: ${{ github.actor }}
install_wheel: true
docker_password: ${{ secrets.GITHUB_TOKEN }}
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 could not move the docker_password parameter to be filled by default since secrets are not available inside of Github custom Actions.

Copy link
Contributor

Choose a reason for hiding this comment

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

have we tried using
secrets: inherit?
https://docs.github.com/en/actions/sharing-automations/reusing-workflows#using-inputs-and-secrets-in-a-reusable-workflow

Seem strange that a github action can't get access to custom GH actions

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, I have tried and the secrets are inaccessible in the Github actions - see here

https://docs.github.com/en/actions/security-for-github-actions/security-guides/using-secrets-in-github-actions#accessing-your-secrets

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes this is true - the secrets context in general is not available in a composite action. You explictly must pass it

dimitri-tenstorrent
dimitri-tenstorrent commented Sep 17, 2024
View reviewed changes
.github/actions/docker-run/action.yml
@@ -65,4 +68,8 @@ runs:
cp -r /github_workspace/* /usr/app/
cd /usr/app/
rm -rf tt_metal tt_eager
if [ ${{ inputs.install_wheel }} ]; 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.

Did we mention in the meeting if we wanted to move the artifact downloading into the docker run action?

I do not know if that makes sense but I also do not like the dependency on the wheel presence that is implicit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

what artifact?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The wheel artifact. This action relies on its presence but there is no explicit dependency defined in the Github Acton jobs.

@dimitri-tenstorrent dimitri-tenstorrent force-pushed the dimitri/12775-clean-up-the-docker-run-action branch from d796e68 to 5a0ad02 Compare September 17, 2024 17:36
@dimitri-tenstorrent dimitri-tenstorrent force-pushed the dimitri/12775-clean-up-the-docker-run-action branch from 434e7b2 to ccbcf98 Compare September 19, 2024 19:17
@dimitri-tenstorrent
#12775: Cleanup docker run action
3b80666 
   - Remove unnecessary arch parameter of the docker run action
   - Add a parameter to installing the wheel
   - Remove LD_LIBRARY env variable since it is not used in the wheel environments
   - Remove Github username since it is accessible inside of the custom Action
   - Incorproate the new learning of not needing to delete the folders and also mount the required files for permissions
   - Set HOME variable so that the packagaes would not be installed in the /home/ubuntu/.local folder
@dimitri-tenstorrent dimitri-tenstorrent force-pushed the dimitri/12775-clean-up-the-docker-run-action branch from 82876fe to 3b80666 Compare September 24, 2024 15:18
@dimitri-tenstorrent dimitri-tenstorrent merged commit cef5730 into main Sep 24, 2024
6 checks passed
@dimitri-tenstorrent dimitri-tenstorrent deleted the dimitri/12775-clean-up-the-docker-run-action branch September 24, 2024 15:19
@Dimitri-Gnidash Dimitri-Gnidash mentioned this pull request Sep 24, 2024
Closed
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

@TT-billteng TT-billteng TT-billteng approved these changes

@ttmchiou ttmchiou Awaiting requested review from ttmchiou ttmchiou is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dimitri-tenstorrent @tt-rkim @TT-billteng @ttmchiou