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

Cache docker builds #3659

Merged
merged 9 commits into from
Oct 25, 2020
Merged

Cache docker builds #3659

merged 9 commits into from
Oct 25, 2020

Conversation

ydcjeff
Copy link
Contributor

@ydcjeff ydcjeff commented Sep 25, 2020

What does this PR do?

Fixes #3637 (comment)

Used docker/build-push-action@v2 to able to cache

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?
  • Did you verify new and existing tests pass locally with your changes?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@ydcjeff ydcjeff marked this pull request as draft September 25, 2020 15:59
@codecov
Copy link

codecov bot commented Sep 25, 2020

Codecov Report

Merging #3659 into master will increase coverage by 3%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #3659    +/-   ##
=======================================
+ Coverage      90%     93%    +3%     
=======================================
  Files         111     111            
  Lines        8011    8011            
=======================================
+ Hits         7217    7448   +231     
+ Misses        794     563   -231     

@ydcjeff ydcjeff changed the title Cache docker builds Cache docker builds [CI SKIP] Sep 25, 2020
@ydcjeff ydcjeff changed the title Cache docker builds [CI SKIP] Cache docker builds Sep 30, 2020
@ydcjeff ydcjeff changed the title Cache docker builds [blocked by #3739]Cache docker builds Sep 30, 2020
@ydcjeff
Copy link
Contributor Author

ydcjeff commented Sep 30, 2020

@Borda I used docker/build-push-action@v2 to cache docker builds (can cache locally or on registry).
And we only cache base-cuda and base-xla builds in nightly events, since
caching in base-conda took more time than no cache, so no cache right now.

Also, storing cache locally took more time when restoring the cache.
I have tested in the forked repo and restoring local cache took ~3 mins while
restoring from registry cache took ~2s, idk why.

So, does pushing cache images (separate images) to docker hub sound good to you?

@ydcjeff ydcjeff marked this pull request as ready for review September 30, 2020 12:14
@mergify mergify bot requested a review from a team September 30, 2020 12:14
@ydcjeff ydcjeff changed the title [blocked by #3739]Cache docker builds Cache docker builds Sep 30, 2020
@mergify
Copy link
Contributor

mergify bot commented Sep 30, 2020

This pull request is now in conflict... :(

1 similar comment
@mergify
Copy link
Contributor

mergify bot commented Sep 30, 2020

This pull request is now in conflict... :(

@ydcjeff ydcjeff mentioned this pull request Oct 2, 2020
7 tasks
@mergify
Copy link
Contributor

mergify bot commented Oct 2, 2020

This pull request is now in conflict... :(

@Borda Borda added the ci Continuous Integration label Oct 2, 2020
@mergify mergify bot requested a review from a team October 2, 2020 10:14
@ydcjeff ydcjeff changed the title Cache docker builds Cache docker builds [CI SKIP] Oct 2, 2020
@ydcjeff
Copy link
Contributor Author

ydcjeff commented Oct 2, 2020

@Borda Shall we install nvidia-dali in this PR or in a later PR?

@Borda
Copy link
Member

Borda commented Oct 2, 2020

@Borda Shall we install nvidia-dali in this PR or in a later PR?

I would keep it paired with the adding DALI example...

@ydcjeff ydcjeff changed the title Cache docker builds [CI SKIP] Cache docker builds Oct 2, 2020
@ydcjeff ydcjeff requested review from Borda and removed request for a team October 2, 2020 15:22
@mergify mergify bot requested a review from a team October 2, 2020 15:22
@edenlightning
Copy link
Contributor

hi @ydcjeff mind taking a look at failing tests?

@Borda Borda added the feature Is an improvement or enhancement label Oct 19, 2020
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

lgtm, just wait till all CI passes

@mergify mergify bot requested a review from a team October 19, 2020 21:40
@Borda Borda added this to the 1.0.x milestone Oct 20, 2020
@ydcjeff
Copy link
Contributor Author

ydcjeff commented Oct 20, 2020

@Borda Before merging, just want to make sure

this PR stores the cache metadata of the docker builds on docker hub with the separate names.
I found caching on docker hub is faster than caching in CI
Does it sounds good to you?

@Borda
Copy link
Member

Borda commented Oct 20, 2020

this PR stores the cache metadata of the docker builds on docker hub with the separate names.

how does it appear on Dockerhub, as a new entry/tag?

@ydcjeff
Copy link
Contributor Author

ydcjeff commented Oct 20, 2020

this PR stores the cache metadata of the docker builds on docker hub with the separate names.

how does it appear on Dockerhub, as a new entry/tag?

It will appear as a new tag like base-cuda-cache-py3.8-torch1.6 (for eg), DIGEST would be UNKNOWN, OS/ARCH will be none, there will be no compressed size.

You can see here for example: https://hub.docker.com/r/ydcjeff/pl/tags (this is just for testing this PR)

@edenlightning edenlightning modified the milestones: 1.0.3, 1.1 Oct 20, 2020
Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

Great addition ! I didn't know about cache-from and cache-to. Really lean API from docker/build-push-action@v2 :) You have some small typos in the comment too.

ydcjeff added 6 commits October 25, 2020 10:40
author ydcjeff <ydcjeff@outlook.com> 1601049378 +0630
committer ydcjeff <ydcjeff@outlook.com> 1601469495 +0630

cache docker builds

lock horovod at 0.19.5

done [ci skip] [CI SKIP]

use --cache-from [ci skip]

typo and horovod [ci skip]

exclude pt 1.3 py3.8 [ci skip]

conda no cache [ci skip]

fix
@Borda Borda added the ready PRs ready to be merged label Oct 25, 2020
@Borda Borda modified the milestones: 1.1, 1.0.x Oct 25, 2020
@ydcjeff ydcjeff removed the ready PRs ready to be merged label Oct 25, 2020
@ydcjeff ydcjeff added the ready PRs ready to be merged label Oct 25, 2020
@ydcjeff
Copy link
Contributor Author

ydcjeff commented Oct 25, 2020

I know 'Ready to go' is not meant to be added by PR author. But fix build-args commit changes small, so I think it's good to go.

@ydcjeff ydcjeff merged commit d83c4e4 into Lightning-AI:master Oct 25, 2020
@ydcjeff ydcjeff deleted the docker/cache branch October 25, 2020 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous Integration feature Is an improvement or enhancement ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants