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.

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

[Docker] - Refactor and automatic upload of containers to repo #486

Merged
merged 38 commits into from Dec 12, 2022

Conversation

goliaro
Copy link
Collaborator

@goliaro goliaro commented Nov 20, 2022

Description of changes:

This PR refactors the Docker containers, to make the builds more efficient, and enables the user to pass build configs through the use of environment variables (passing build configs to Docker got partially broken after #392 ). In addition, we add a mechanism to automatically publish the environment and flexflow containers to ghrc.io. The CUDA version of the uploaded docker image is compatible with any CUDA GPU architecture. This gives us several advantages:

  • users can just pull the flexflow docker image if they want to try flexflow under the default configs
  • CI tests can use the flexflow-environment image to avoid having to re-install all FlexFlow dependencies in each test

Related Issues:

Linked Issues:

  • Issue #

Issues closed by this PR:

  • Closes #

Before merging:

  • Did you update the flexflow-third-party repo, if modifying any of the Cmake files, the build configs, or the submodules?

@lockshaw lockshaw marked this pull request as ready for review December 9, 2022 22:51
@goliaro goliaro changed the title [Docker] - Pull/Push from Github container repository [Docker] - Refactor and automatic upload of containers to repo Dec 11, 2022
Copy link
Collaborator

@lockshaw lockshaw left a comment

Choose a reason for hiding this comment

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

Overall LGTM.

If the bash scripts in docker/ continue to get more complicated, we should really consider porting them over to python. Just having argparse, etc. would probably already reduce the amount of code significantly, and remove a bunch of more complex and unusual bash features.

docker/build.sh Outdated Show resolved Hide resolved
docker/build.sh Outdated Show resolved Hide resolved
docker/run.sh Show resolved Hide resolved
goliaro and others added 5 commits December 11, 2022 18:46
Co-authored-by: Colin Unger <lockshaw@lockshaw.net>
Co-authored-by: Colin Unger <lockshaw@lockshaw.net>
@goliaro
Copy link
Collaborator Author

goliaro commented Dec 12, 2022

Overall LGTM.

If the bash scripts in docker/ continue to get more complicated, we should really consider porting them over to python. Just having argparse, etc. would probably already reduce the amount of code significantly, and remove a bunch of more complex and unusual bash features.

yes, good point. Perhaps we can switch to Python when adding the pytorch align tests in CI

@goliaro goliaro enabled auto-merge (squash) December 12, 2022 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants