[VPEX][5/8] Add local-env pipeline, detection, and package-manager interface#5828
Open
rugpanov wants to merge 4 commits into
Open
[VPEX][5/8] Add local-env pipeline, detection, and package-manager interface#5828rugpanov wants to merge 4 commits into
rugpanov wants to merge 4 commits into
Conversation
Contributor
Waiting for approvalCould not determine reviewers from git history. Eligible reviewers: Suggestions based on git history. See OWNERS for ownership rules. |
Collaborator
Integration test reportCommit: d8f0de9
21 interesting tests: 14 SKIP, 7 RECOVERED
Top 5 slowest tests (at least 2 minutes):
|
rugpanov
force-pushed
the
dbconnect/04-merge
branch
from
ccc19f1 to
92f1dda
Compare
rugpanov
force-pushed
the
dbconnect/05-pipeline
branch
from
6289ebf to
b94edd5
Compare
rugpanov
changed the title
rugpanov
force-pushed
the
dbconnect/04-merge
branch
from
92f1dda to
b96bf8b
Compare
rugpanov
force-pushed
the
dbconnect/05-pipeline
branch
from
b94edd5 to
dcdd6f3
Compare
rugpanov
force-pushed
the
dbconnect/04-merge
branch
from
b96bf8b to
6477a4c
Compare
rugpanov
force-pushed
the
dbconnect/05-pipeline
branch
from
dcdd6f3 to
aa3fa6f
Compare
rugpanov
force-pushed
the
dbconnect/04-merge
branch
from
6477a4c to
f4cc1c7
Compare
rugpanov
force-pushed
the
dbconnect/05-pipeline
branch
from
aa3fa6f to
965e3d0
Compare
rugpanov
force-pushed
the
dbconnect/05-pipeline
branch
from
965e3d0 to
d6aeed6
Compare
rugpanov
force-pushed
the
dbconnect/04-merge
branch
from
56b496d to
7b23583
Compare
rugpanov
force-pushed
the
dbconnect/05-pipeline
branch
from
b7c9e6d to
14d8d17
Compare
rugpanov
force-pushed
the
dbconnect/04-merge
branch
from
7b23583 to
a1644d9
Compare
rugpanov
force-pushed
the
dbconnect/05-pipeline
branch
from
14d8d17 to
c010a22
Compare
Fifth in the stacked local-env series. This completes the libs/localenv engine; the uv backend and the CLI command follow in later PRs. - pipeline.go: the six-phase orchestrator (preflight → resolve → fetch → merge → provision → validate) that ties the earlier layers together and records per-phase status into the Result. It drives everything through the PackageManager interface, so it is unit-tested end to end against a fake package manager and a stub compute client. This file also owns the filesystem/artifact constants and the canonical phase-order slice, which the foundation PR intentionally left to their consumer. - pkgmanager.go: the PackageManager interface the pipeline provisions through (implemented by uv in the next PR). - detect.go: package-manager detection (uv vs conda vs pip) and the writability preflight, biased toward uv since uv's native project file is the pyproject.toml this command already merges. A read error on an existing pyproject.toml that is not a not-exist error is treated as a failure rather than as greenfield, so a permission or transient I/O error can never cause the project to be overwritten with a fresh file and no backup. Depends on the foundation, target, constraints, and merge PRs. The package is still dormant: nothing under cmd/ imports it. Co-authored-by: Isaac
Review of the pipeline layer found: - applyMerge treated every os.Stat error on the backup as "does not exist" and proceeded to copyFile over it. An existing-but-unstattable backup (permission or I/O error) would thus be overwritten with the already-merged content, destroying the canonical original the merge base relies on. It now only creates the backup on os.ErrNotExist and fails on any other stat error. - A backup-copy failure was reported with DiskMutated=false even though copyFile creates/truncates the .bak path and can leave a partial file. That path now reports DiskMutated=true. - majorVersion accepted any non-empty prefix before the first dot, so a malformed version like "bad.version" yielded major "bad" and could be compared as a real major. It now returns "" for a non-numeric major so the validate phase rejects malformed versions. Adds a direct applyMerge test for the unstattable-backup branch and extends the majorVersion cases with non-numeric inputs. Co-authored-by: Isaac
Round-4 review noted preflight ran ensureWritable even under --check, which creates and removes a temp file (a disk mutation in a dry run) and fails a read-only project the user only wants to inspect. The probe now runs only for a real (non-check) run; it exists to fail fast before a write that --check never performs. Co-authored-by: Isaac
rugpanov
force-pushed
the
dbconnect/04-merge
branch
from
a1644d9 to
79113e4
Compare
rugpanov
force-pushed
the
dbconnect/05-pipeline
branch
from
c010a22 to
27e56f9
Compare
Round-5 review noted that although --check now skips the writability probe, it still called PackageManager.EnsureAvailable, which may install the manager (uv) when missing — another disk mutation in a dry run. Preflight now performs neither write-side step under --check (neither is needed to compute the plan) and reports the phase as "check". Adds a test with a PackageManager that errors on every method, asserting --check still succeeds and produces a plan. Co-authored-by: Isaac
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
2 participants
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.
Context
PR 5 of 8 in the stacked
databricks local-env python syncseries (see #5823 for the full plan). Stacked on #5827 — review #5823, #5824, #5826, #5827 first; this PR's diff is the five files below. This PR completes thelibs/localenvengine; the uv backend and the CLI command follow in PRs 6–8.What this PR contains
pipeline.go— the six-phase orchestrator (preflight → resolve → fetch → merge → provision → validate) that ties the earlier layers together and records per-phase status into theResult. It drives everything through thePackageManagerinterface, so it's unit-tested end-to-end against a fake package manager and a stub compute client. This file also owns the filesystem/artifact constants and the canonical phase-order slice that the foundation PR intentionally left to their consumer.pkgmanager.go— thePackageManagerinterface the pipeline provisions through (implemented by uv in the next PR).detect.go— package-manager detection (uv vs conda vs pip) and the writability preflight, biased toward uv since uv's native project file is thepyproject.tomlthis command already merges.Worth close review
A read error on an existing
pyproject.tomlthat is not a not-exist error is treated as a failure rather than as greenfield — so a permission change or transient I/O error can never cause the project to be overwritten with a fresh file and no backup (the!greenfieldpath is the only one that takes a backup).Dependencies & surface
Depends on the foundation, target, constraints, and merge PRs (#5823/#5824/#5826/#5827). Still dormant — nothing under
cmd/imports the package.This pull request and its description were written by Isaac.