Reapply "feat: move from two-track to three-track container build (#115)" (#126)#138
Reapply "feat: move from two-track to three-track container build (#115)" (#126)#138
Conversation
There was a problem hiding this comment.
Pull request overview
This PR reapplies the three-track container build approach from PR #115, which was previously reverted in PR #126 due to image bloat issues. The key fix is changing builder_concretization_custom to derive from builder_concretization_default instead of builder_installation_default, preventing the custom stage from inheriting installed packages and causing a full rebuild.
Changes:
- Updated documentation to describe three-track build approach (concretization track, installation track, runtime track)
- Restructured Dockerfile stages to separate concretization from installation in the builder track
- Modified CI workflow to test builder_concretization_custom stage earlier for cuda and tf environments
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| docs/architecture.md | Updated mermaid diagram and description from two-track to three-track build approach, showing separate concretization and installation tracks |
| containers/eic/Dockerfile | Changed builder_concretization_custom to derive from builder_concretization_default (line 150), consolidated runtime stages by removing intermediate concretization/installation split |
| .github/workflows/build-push.yml | Updated target from builder_concretization_default to builder_concretization_custom for cuda and tf environments to enable earlier concretization testing |
| ## builder track: runtime track: | ||
| ## concretization: installation: concretization/installation: | ||
| ## --------------------------------------------------------------------------------------- | ||
| ## builder_image runtime_image | ||
| ## builder_concretization_default | ||
| ## builder_installation_default -> runtime_concretization_default (copy spack.lock) | ||
| ## \-> runtime_installation_default (from buildcache) | ||
| ## builder_concretization_custom | ||
| ## builder_installation_custom -> runtime_concretization_custom (copy spack.lock) | ||
| ## \-> runtime_installation_custom (from buildcache) | ||
| ## \-> builder_installation_default | ||
| ## runtime_default | ||
| ## (copy spack.lock from builder_installation_default) | ||
| ## (install via buildcache) | ||
| ## \-> builder_concretization_custom | ||
| ## \-> builder_installation_custom | ||
| ## \-> runtime_custom | ||
| ## (copy spack.lock from builder_installation_custom) | ||
| ## (install via buildcache) |
There was a problem hiding this comment.
The ASCII diagram is misleading about the dependency structure. Line 30 shows ## \-> builder_concretization_custom which suggests it derives from builder_image, but it actually derives from builder_concretization_default (as shown in line 150). The arrow should be indented to show it's a continuation from builder_concretization_default on line 25, similar to how builder_installation_default is shown on line 26.
1 participant
Briefly, what does this PR introduce?
This PR reverts commit 161a8ec in #126, and reapplies commit 4e49b9c in #115.
What kind of change does this PR introduce?