Skip to content

Conversation

@GMNGeoffrey
Copy link
Contributor

@GMNGeoffrey GMNGeoffrey commented Sep 3, 2020

Refactors Docker setup to enable multistage builds and fully support
Vulkan.

  1. Vulkan is now supported in all our build configurations and
    IREE_VULKAN_DISABLE is set to false in all build scripts.
  2. Our docker containers use multi-stage builds to reduce duplication
    and image size.
  3. Directory names match image names rather than having arbitrary
    underscore/dash distinction (I kept mistyping this).

New vs old images:
https://gist.github.com/GMNGeoffrey/6ea51a6035f3eb8fac11ff4bdbd6edc0

Fixes #2651

@google-cla google-cla bot added the cla: yes label Sep 3, 2020
GMNGeoffrey added a commit that referenced this pull request Sep 3, 2020
Otherwise the diff will list deleted files and then grep will fail
because the files don't exist, e.g.
https://github.com/google/iree/pull/3065/checks?check_run_id=1068022415

Tested:
Ran the script before (failing) and after (succeeding) on
#3065, which deletes files.
@GMNGeoffrey GMNGeoffrey marked this pull request as ready for review September 3, 2020 22:11
@GMNGeoffrey GMNGeoffrey requested review from antiagainst and phoenix-meadowlark and removed request for antiagainst September 3, 2020 22:11
@GMNGeoffrey
Copy link
Contributor Author

This got a bit bigger than I expected. I was fiddling around with multistage builds and didn't really see it come together until everything worked. I can break it into smaller chunks if you'd like.

@GMNGeoffrey
Copy link
Contributor Author

Argh rollout will actually be tricky because the image names change :-/ I think we should switch to using sha256 so we can have atomic changes and just use the tags for caching. Docker caches based on layers, so it should only be a minimal performance hit as it realizes it already has all the layers. I'll test that out in a separate PR, but please take a look at these changes in the meantime :-)

@GMNGeoffrey
Copy link
Contributor Author

This is able to rollout now thanks to #3119. PTAL


# Additionally, we need to install the Vulkan SDK and the NVIDIA Vulkan driver.
# It would be nice to have a separate install vulkan image and copy from that,
# but I don't know all the files that installing the vulkan-sdk adds.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it normal to use pronouns like I, my and mine in OSS code? Probably doesn't matter much either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really specific to OSS, but the appropriate use of the first person is an old argument :-D I personally lean towards it where the alternative is some twisted passive voice or other acrobatics just to avoid the first person. I could also say "we", but I dunno maybe someone else knows how to do this (Lei?). This argument also comes up in academic papers all the time. I think avoiding the first person to seem "professional" or "formal" is a much lower priority than communicating with your reader :-P

I figure blame will usually say who the speaker is, but it usually doesn't actually matter (I wouldn't write "come ask me about this" because then someone actually has to figure out the speaker). This is a note for future devs explaining why something is the way it is and I wrote it in what felt like the most natural way. I quite frequently come across things like this where it's clearly not following what you'd expect the convention would be and I go down the rabbit hole of trying to fix it and sometimes discover the original author had a good reason for not doing so and man it would've been nice if they'd left a note. I'm happy to word it differently though :-D

/rant

@GMNGeoffrey
Copy link
Contributor Author

Whelp good thing I manually triggered that presubmit :-D

@GMNGeoffrey GMNGeoffrey merged commit f941efb into iree-org:main Sep 15, 2020
@GMNGeoffrey GMNGeoffrey deleted the rbe-swiftshader branch September 15, 2020 20:16
@ScottTodd ScottTodd mentioned this pull request Sep 16, 2020
copybara-service bot pushed a commit that referenced this pull request Sep 16, 2020
* 903f809 Script to update to llvm syncpoint (#3155)
* a398c96 Translate VM Function to C Code (#2842)
* f941efb Multistage docker containers with RBE swiftshader support (#3065)
* b7d5710 [spirv] Extract common SPIR-V target functionalities out (NFC) (#3140)
* f5fe759 Use generated function to compute number of workgroups to be launched. (#3095)
* b89191c Merge google -> main (#3143)

COPYBARA_INTEGRATE_REVIEW=#3160 from ScottTodd:main-to-google 903f809
PiperOrigin-RevId: 331891580
ScottTodd added a commit that referenced this pull request Sep 16, 2020
* 903f809 Script to update to llvm syncpoint (#3155)
* a398c96 Translate VM Function to C Code (#2842)
* f941efb Multistage docker containers with RBE swiftshader support (#3065)
* b7d5710 [spirv] Extract common SPIR-V target functionalities out (NFC) (#3140)
* f5fe759 Use generated function to compute number of workgroups to be launched. (#3095)
* b89191c Merge google -> main (#3143)

COPYBARA_INTEGRATE_REVIEW=#3160 from ScottTodd:main-to-google 903f809
PiperOrigin-RevId: 331891580
GMNGeoffrey added a commit that referenced this pull request Sep 18, 2020
These builds have replaced the old x86 builds since
#3065
GMNGeoffrey added a commit that referenced this pull request Sep 19, 2020
…where (#3202)

#3065 added support for swiftshader
everywhere and we've switched over all our builds. These configs are now
unused.
@GMNGeoffrey GMNGeoffrey added the infrastructure Relating to build systems, CI, or testing label Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

infrastructure Relating to build systems, CI, or testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use multistage builds for docker test images

2 participants