Skip to content

Set _F_IMG_ID env var all the way at the bottom during ImageSpec builds#3227

Merged
pingsutw merged 5 commits into
flyteorg:masterfrom
redartera:default-builder-f-img-id-move-down
May 6, 2025
Merged

Set _F_IMG_ID env var all the way at the bottom during ImageSpec builds#3227
pingsutw merged 5 commits into
flyteorg:masterfrom
redartera:default-builder-f-img-id-move-down

Conversation

@redartera
Copy link
Copy Markdown
Contributor

@redartera redartera commented Apr 12, 2025

Why are the changes needed?

Docker build caching is highly sensitive to the order of operations in a Dockerfile. Currently, the _F_IMG_ID environment variable (which contains a hash of the ImageSpec dataclass) is set near the top of the generated Dockerfile. When this value changes due to modifications in the ImageSpec, it invalidates the cache for that layer and all subsequent layers, causing unnecessary rebuilds of the Python environment even when those layers haven't changed functionally (e.g. python dependencies have remained the same but something changed in the commands or copy block following it)

What changes were proposed in this pull request?

This PR moves the _F_IMG_ID environment variable declaration to the bottom of the generated Dockerfile, after all major build steps (like uv/poetry/pip env installations). This placement ensures that changes to the ImageSpec as a whole only invalidate the layers that are directly affected, preventing costly rebuilds of the Python environment when only commands or other non-environment components change.

How was this patch tested?

The change was tested locally by modifying flytekit and observing Docker build behavior. When the _F_IMG_ID is placed at the bottom of the Dockerfile, changes to components like commands only invalidate the cache for those specific layers and what follows them, preserving the cache for expensive operations like Poetry environment setup.

This improvement reduces build times and registry storage space by maintaining slightly better cache consistency, especially for iterative development workflows where small changes shouldn't trigger full environment rebuilds.

Summary by Bito

This PR optimizes Dockerfile generation by repositioning the _F_IMG_ID environment variable to the end of the build process. This strategic change preserves cache layers for steps unaffected by ImageSpec changes, preventing unnecessary rebuilds when only non-critical configuration changes occur. This significantly improves build times, reduces storage requirements, and enhances overall Docker build performance during development.

Unit tests added: False

Estimated effort to review (1-5, lower is better): 1

@flyte-bot
Copy link
Copy Markdown
Contributor

flyte-bot commented Apr 12, 2025

Code Review Agent Run #e7657b

Actionable Suggestions - 0
Review Details
  • Files reviewed - 1 · Commit Range: fb9edb3..fb9edb3
    • flytekit/image_spec/default_builder.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

Refer to the documentation for additional commands.

Configuration

This repository uses code_review_bito You can customize the agent settings here or contact your Bito workspace admin at haytham@union.ai.

Documentation & Help

AI Code Review powered by Bito Logo

@flyte-bot
Copy link
Copy Markdown
Contributor

flyte-bot commented Apr 12, 2025

Changelist by Bito

This pull request implements the following key changes.

Key Change Files Impacted
Feature Improvement - Optimized Docker Build for Improved Caching

default_builder.py - Repositioned _F_IMG_ID environment variable declaration to the bottom of the Dockerfile and adjusted environment variable setup, thereby refining the build caching mechanism to avoid unnecessary rebuilds.

@redartera redartera force-pushed the default-builder-f-img-id-move-down branch from fb9edb3 to 915badb Compare April 12, 2025 14:08
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.20%. Comparing base (1bc8302) to head (4853f0c).

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #3227       +/-   ##
===========================================
+ Coverage   81.52%   93.20%   +11.68%     
===========================================
  Files         343       37      -306     
  Lines       28627     2370    -26257     
  Branches     2935        0     -2935     
===========================================
- Hits        23338     2209    -21129     
+ Misses       4445      161     -4284     
+ Partials      844        0      -844     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@flyte-bot
Copy link
Copy Markdown
Contributor

flyte-bot commented Apr 12, 2025

Code Review Agent Run #0b48aa

Actionable Suggestions - 0
Review Details
  • Files reviewed - 1 · Commit Range: fb9edb3..915badb
    • flytekit/image_spec/default_builder.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

Refer to the documentation for additional commands.

Configuration

This repository uses code_review_bito You can customize the agent settings here or contact your Bito workspace admin at haytham@union.ai.

Documentation & Help

AI Code Review powered by Bito Logo

Signed-off-by: redartera <reda@artera.ai>
@redartera redartera force-pushed the default-builder-f-img-id-move-down branch from 915badb to 195e795 Compare April 15, 2025 13:17
@flyte-bot
Copy link
Copy Markdown
Contributor

flyte-bot commented Apr 15, 2025

Code Review Agent Run #133435

Actionable Suggestions - 0
Review Details
  • Files reviewed - 1 · Commit Range: 915badb..195e795
    • flytekit/image_spec/default_builder.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

Refer to the documentation for additional commands.

Configuration

This repository uses code_review_bito You can customize the agent settings here or contact your Bito workspace admin at haytham@union.ai.

Documentation & Help

AI Code Review powered by Bito Logo

@flyte-bot
Copy link
Copy Markdown
Contributor

flyte-bot commented Apr 29, 2025

Code Review Agent Run Status

  • Limitations and other issues: ❌ Failure - Bito Code Review Skipped - No Changes

    Bito didn't review because we did not see any changes in the PR to review.
    To trigger review again, type /review in the comment and save.
    You can change the settings here, or contact the agent instance creator at eduardo@union.ai.

@flyte-bot
Copy link
Copy Markdown
Contributor

flyte-bot commented May 2, 2025

Code Review Agent Run Status

  • Limitations and other issues: ❌ Failure - Bito Code Review Skipped - No Changes

    Bito didn't review because we did not see any changes in the PR to review.
    To trigger review again, type /review in the comment and save.
    You can change the settings here, or contact the agent instance creator at eduardo@union.ai.

Copy link
Copy Markdown
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

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

Should we just move $ENV to the end of the Dockerfile? cc @thomasjpfan

@thomasjpfan
Copy link
Copy Markdown
Contributor

Should we just move $ENV to the end of the Dockerfile?

It's hard to say, if one sets any of the UV environment variables, then you'll want the environment variables before the UV install command.

@redartera
Copy link
Copy Markdown
Contributor Author

redartera commented May 4, 2025

Should we just move $ENV to the end of the Dockerfile?

It's hard to say, if one sets any of the UV environment variables, then you'll want the environment variables before the UV install command.

+1 to @thomasjpfan - I preferred to only move _F_IMG_ID because it doesn't seem used during the build but rather at runtime

Signed-off-by: redartera <reda@artera.ai>
@flyte-bot
Copy link
Copy Markdown
Contributor

flyte-bot commented May 4, 2025

Code Review Agent Run #b6be55

Actionable Suggestions - 0
Review Details
  • Files reviewed - 1 · Commit Range: 195e795..02c6513
    • flytekit/image_spec/default_builder.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

Refer to the documentation for additional commands.

Configuration

This repository uses code_review_bito You can customize the agent settings here or contact your Bito workspace admin at haytham@union.ai.

Documentation & Help

AI Code Review powered by Bito Logo

@flyte-bot
Copy link
Copy Markdown
Contributor

flyte-bot commented May 6, 2025

Code Review Agent Run Status

  • Limitations and other issues: ❌ Failure - Bito Code Review Skipped - No Changes

    Bito didn't review because we did not see any changes in the PR to review.
    To trigger review again, type /review in the comment and save.
    You can change the settings here, or contact the agent instance creator at eduardo@union.ai.

@pingsutw pingsutw merged commit 0ef2287 into flyteorg:master May 6, 2025
115 checks passed
Atharva1723 pushed a commit to Atharva1723/flytekit that referenced this pull request Oct 5, 2025
…builds (flyteorg#3227)

Signed-off-by: redartera <reda@artera.ai>
Signed-off-by: Atharva <atharvakulkarni172003@gmail.com>
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.

4 participants