Repository navigation
Ship Dockerfile.base in the wheel and drop a dead package_data entry - #2743
Open
Oke Iyanuoluwa Enoch (Iyanuoluwa007) wants to merge 2 commits into
Open
Oke Iyanuoluwa Enoch (Iyanuoluwa007) wants to merge 2 commits into
Oke Iyanuoluwa Enoch (Iyanuoluwa007) wants to merge 2 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Docker packaging (packaging_generator) reads olive/engine/packaging/Dockerfile.base at runtime, but setup.py never listed it in package_data, so it was missing from built wheels and Docker packaging failed for pip installs. Add it. Also remove the olive.auto_optimizer entry: that package no longer exists. Add a test that every package_data entry is a real package and every data file under olive/ is covered, except the cost model CSVs, which are only linked from the docs. Part of microsoft#2516
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The resource-coverage check accepts incorrectly rooted patterns that would leave files out of the wheel.
1 open issue
What changed in this PR
Fixes Docker packaging for wheel-installed Olive by including its required Dockerfile.
Changes:
- Adds
Dockerfile.baseto package data. - Removes the obsolete
olive.auto_optimizerentry. - Adds checks for package existence and resource coverage.
| File | Description |
|---|---|
test/test_package_data.py |
Adds package-data regression checks. |
setup.py |
Includes the Dockerfile and removes stale metadata. |
Review effort: Balanced
Path.match only compares trailing path components, so a pattern declared under the wrong package (or a *.yaml pattern that should not cover nested files) still passed. Expand each pattern with package_dir.glob, like setuptools does, and add a test for a pattern rooted at the wrong package.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Describe your changes
The codebase review (#2516) flagged a dead
olive.auto_optimizerentry insetup.py'spackage_data. While checking that, I found a real packaging bug next to it.Dockerfile.baseis missing from the wheel.olive/engine/packaging/packaging_generator.pyopensPath(__file__).parent / "Dockerfile.base"when packaging a workflow output as Docker, butsetup.pynever listed that file inpackage_data(andinclude_package_data=False). I built a wheel frommainwithpip wheel . --no-deps, and the file isn't in it, so Docker packaging fails withFileNotFoundErrorfor anyone who installedolive-aifrom a wheel. It now ships underolive.engine.packaging.Dead entry.
olive.auto_optimizerno longer exists, so itspackage_dataentry is removed.Regression test.
test/test_package_data.pyparsespackage_datafromsetup.pywithastand checks that:olive/is covered by a pattern.The only exception is
olive/assets/cost_models/*.csv, which the docs link to on GitHub as examples and the code never loads. Both tests fail onmainand pass with this change.I left the uninstallable
tfextra (tensorflow==1.15.0) from the review alone, since removing it is a product decision.Checklist before requesting a review
pytest test/test_package_data.py: 2 passed. Rebuilt the wheel and confirmedolive/engine/packaging/Dockerfile.baseis now included.)lintrunner -a(ruff 0.15.11 check and format on changed files)Fixed Docker packaging failing for wheel installs because
Dockerfile.basewas not included in the package.(Optional) Issue link
Part of #2516