Repository navigation
Conversation
Change mapRequirement: read scoped Unity Catalog Volume Parquet inputs through the existing streaming reader. Rewrites previously reused constant or size-only versions, empty inputs differed from SQL sources, and the data-only extra installed MLflow vendor clients.
Verification: 67 provider/backend/Volume/source tests and 69 packaging regressions passed. Independent review checked every in-scope Vedaant numbered/smaller request. Managed Databricks workflow 42938850179 passed all four engine phases and cleanup. Lock metadata was regenerated with uv 0.13.7. Stopping condition: metadata changes invalidate source versions, mid-read changes discard staging, empty/unsupported/out-of-scope inputs fail clearly, and actual built wheel extras preserve the data-only boundary. No snapshot-load behavior or native platform UI is claimed. |
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
|
Vedaant-J
left a comment
There was a problem hiding this comment.
The title says "restore Databricks snapshots", but snapshot restore (GraphSnapshot.load_from_connector, _snapshot_io) isn't touched. What the PR adds is reading a /Volumes/<catalog>/<schema>/<volume>/... file or folder as a Databricks source through the existing streaming reader. Please retitle. Reusing the shared reader is the right approach (parallel ranged reads, cleanup on failure, no new Python pass). The version handling needs fixing before merge:
- A single Volume file always gets version
"-1".volume_filesystemdropslast_modified, so_source_versionreturns the constantSourceVersion("-1"). I rewrote a file once with the same size and once with different rows; the version stayed the same both times. Because"-1"isn'tNone,TrainingTable._table_idgives the samett-…id after a rewrite, and_storeskips the upload because_SUCCESSexists. The artifact root keeps the old rows while the new model's lineage points at them. The same constant goes intoGraph.snapshot'sdata_version, so an old index is reused. The SDK exposesDirectoryEntry.last_modified/GetMetadataResponse.last_modified; please put mtimes into the listing (or pass anobject_identityfromget_metadata) and add a test that rewrites a file and expects a new version. - Folder versions hash only path and size, but are labelled
listing, whichsource_changestreats as comparable. A same-size rewrite reportsunchanged, and a concurrent same-size rewrite isn't caught by the changed-during-read check (its mtime isNone). Same fix as 1. - Empty inputs. The SQL route raises
PermanentSourceError(... no rows), but the Volume route accepts a zero-row Parquet file. An empty folder raises a bareIndexError(with dtypes) orValueError("a staged table requires at least one path…"). Callingread_table(reader, '', dest, columns=dtypes, mode=self.mode, sql=sql, version=data_version)and sharing the existing empty-table tail fixes this, and removes the hand-written mode logic and the secondrequire_no_sql. - Volume paths only half work.
get_columns,get_sampleandget_table_configstill treat the path as a SQL table and fail with "does not match the connector catalog". Either route those too, or document that Volume paths are for training tables and custom sources only, with a clear error. - Connector scope is bypassed. A connector set to catalog
a/ schemabreads/Volumes/other/..., and a bare volume root lists the whole volume recursively. Unity Catalog still enforces permissions, but it's inconsistent with the SQL path and undocumented. - Please move the
mlflow-skinny[databricks]dependency to its own PR or the mlflow extra. It pullsdatabricks-agents,tiktoken, Azure/GCS clients and boto3 into every data-onlykumo-engine[databricks]install (uv.lock+263). The new wheel test rebuilds the requirements by hand, so it mostly tests marker handling rather than the real build.
Smaller:
test_volume_parquet_ingest_preserves_training_rowscompares"-1"with"-1", so it passes trivially.test_invalid_volume_coordinate_never_opens_a_clientexercises the existing SQL-name check, not new code.data_versionlists hidden files butpreparedoesn't, so an in-progress_temporary/write makes them disagree and the source reportschanged.- The README says only same-size rewrites go undetected; for single files every rewrite does.
- Unrelated churn in the faiss test and
test_backend.py. - The test catalog and volume names match the live harness; neutral fake names would be safer in a public repo.
…bricks-volume-restore
…bricks-volume-restore
…bricks-volume-restore
…bricks-volume-restore
…bricks-volume-restore
…bricks-volume-restore
…bricks-volume-restore
|
Summary
Read Parquet files and folders below a Unity Catalog Volume through the existing streaming source reader. This adds source-path support; it does not change snapshot loading.
databricksextra data-only. Vendor MLflow dependencies belong tomlflow, verified through an actual setuptools metadata build and wheel merge.Change map
Maintained change map
Testing Done