Skip to content

Ingestion v2 - config outpath, write registry and validation, write utils, single write_models - #5

Open
reneyagmur wants to merge 43 commits into
mainfrom
ingestion-v2
Open

Ingestion v2 - config outpath, write registry and validation, write utils, single write_models#5
reneyagmur wants to merge 43 commits into
mainfrom
ingestion-v2

Conversation

@reneyagmur

Copy link
Copy Markdown
Collaborator

IO layer: write path + validation

Ships the curated connects_common_connectivity.io write path end-to-end: package-wide configuration, a registry-driven write API, write-time validation derived from that same registry, ETL notebook migration to the new API, and the test suite to back it.

Design: WriteSpec as the single source of truth

The WriteSpec registered per writable class is one declaration that drives both Delta dispatch (subdir, partitioning, scope columns, write mode) and write-time validation (required_for_write slots are flipped non-optional in auto-derived strict submodels and re-validated before any IO). Generated models.py is never touched.

Configuration

  • New connects_common_connectivity.config: pydantic Settings, cached get_settings(), walk-up discovery of ccc_config.yaml, plus output_root() / table_path() helpers. Relative values anchor at the config file's directory via os.path.abspath (avoids Code Ocean's scratch -> /scratch symlink).
  • Precedence: explicit arg > CCC_OUTPUT_ROOT env > ccc_config.yaml > error.
  • Repo-root ccc_config.yaml seeded.

Write registry and dispatch

  • io/write_spec.py: WriteSpec, REGISTRY (14 entries), get_spec().
  • io/writers.py: write_models() single-dispatch over the registry (no per-class wrappers), frozen WriteResult dataclass, WRITABLE_CLASSES tuple. write_projection_matrix() is the only non-write_models writer so far, justified by its non-uniform signature (dense matrix + model).
  • populate_region_coverage() added in io/write_utils.py; derives region_coverage from the dense values before write.
  • DataSet scope widened to (project_id, id) so patchseq exc/inh DataSet rows coexist (today's predicate-only-on-project_id behavior would overwrite one with the other).

Write-time validation

  • io/write_validation.py: strict_model_for(cls) flips WriteSpec.required_for_write slots to non-optional and strips Optional from those annotations (cached per class, no mutation of generated models.py). validate_for_write() re-validates instances and raises ValueError naming the missing slots before any IO. Wired into write_models.
  • required_for_write populated for Cluster, ClusterMembership, CellFeatureDefinition.

Public API surface

  • Curated io/__init__.py re-exports pinned by __all__: get_settings, Settings, table_path, write_models, write_projection_matrix, WriteResult, WRITABLE_CLASSES.
  • Per-call output_root= keyword on write_models() / write_projection_matrix() (mutually exclusive with settings=) so a single notebook can redirect its writes without mutating process-global config.
  • Modality.CALCIUM_IMAGING added (for functional correlations in microns or v1dd-like datasets with EM + CI experiments).
  • Removed connects_common_connectivity.arrow_utils / connects_common_connectivity.write_utils re-export shims; arrow_utils.py and write_utils.py now live exclusively under io/.

ETL notebook migration

  • Every registry-backed class is now exclusively written through write_models / write_projection_matrix in the ETL notebooks. Hand-rolled write_deltalake migrated. Per-notebook imports trimmed.
  • Hardcoded OUTPUT_ROOT = "../scratch/..." strings replaced with output_root().
  • Patchseq exc/inh regression covered (see DataSet scope fix above).

Tests

  • Shared tests/conftest.py foundations (settings/cache/cwd isolation + shared fixtures); duplicated helpers removed.
  • Tightened exception assertions to specific classes with meaningful match= checks.
  • High-signal regression assertion messages where failures are otherwise hard to diagnose; list-validation failures now include row context.
  • Per-class smoke parametrized over WRITABLE_CLASSES; registry-drift guard; no-shim regression (test_shim_modules_deleted, _not_importable, _no_source_references_shim_paths).
  • Closed coverage gaps: CLI behavior, parquet loader contract, predicate escaping edge cases, relocation scan roots, dry-run semantics.
  • Patchseq regression, idempotency, append-new-by-id, predicate construction, output_root= override, strict-validation failures, public-API surface.

Not in this PR

  • Wide cell-feature / projection-matrix parquet writes (still use write_deltalake directly).
  • CellCellConnectivityLong — no registry entry yet; the write_cellcellconnectivitylong stub in io/writers.py documents the migration plan.
  • The etl_v1dd_01 new dataset ingestion prototype ongoing in parallel.
  • A merge_by_id (read-existing → union → overwrite) write mode for shared scopes like (visp_patchseq, visp_inh_patchseq) where multiple notebooks contribute disjoint subsets. The union is currently inlined in patch-seq / WNM notebooks; see planning/multi_writer_scope_design.md for the draft design discussion.

Verification

uv run pytest -q → 160 passed.

@danielsf danielsf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks pretty good.

Mostly there are some convention/documentation changes that I think would help make the code easier to maintain in the future.

Comment thread src/connects_common_connectivity/config.py Outdated
Comment thread src/connects_common_connectivity/io/write_utils.py Outdated
Comment thread src/connects_common_connectivity/io/write_validation.py Outdated
Comment thread src/connects_common_connectivity/io/write_validation.py Outdated
Comment thread src/connects_common_connectivity/io/write_validation.py Outdated
Comment thread src/connects_common_connectivity/io/writers.py Outdated
Comment thread tests/test_config.py Outdated
Comment thread tests/test_config.py
Comment thread tests/test_parquet_loader.py
Comment thread ccc_config.yaml
@@ -0,0 +1,5 @@
# Package-wide settings for ConnectsCommonConnectivity.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Making this config file a part of the repository is going to create problems for users. Every time they edit this file, it will look as if they are editing the code base and could cause collisions if they do a git pull (or start working on their own branches).

Can we not include this in the repository (especially given how basic this file is) and just add a section to the README.md explaining how to create a config.yaml file?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

can i keep it for now and exclude it closer to wider adoption? i kind of want to keep track of the output data versions wrt git history.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

thanks for bringing this up!

in the future i will separate this repo into 3 repos:

  1. schema only package
  2. schema io functions package
  3. demo notebooks

io functions will have init config type of helper functions and a readme. demo notebooks will have an example config file.

for now it makes sense to me that config file data outpath versions stay together with the notebooks. and nobody else pushes to main yet so config file edits wont be a problem.

@reneyagmur

Copy link
Copy Markdown
Collaborator Author

Hi Scott, thanks a lot for the comments they were the most helpful

Here are the changes (human reviewed AI summary based on the commits that were based on the changes made in response to the comments):

Imports and public API cleanup

  • Moved runtime and test imports to module scope and converted package-relative
    imports to absolute connects_common_connectivity... imports. The relocation
    tests retain dynamic imports only where they intentionally assert that removed
    modules cannot be imported. (3790a39, a0dcae7)
  • Renamed the result dataclass and all public exports, annotations, and tests
    from WriteResult to WrittenResult, reflecting that it describes an
    already-completed write. (a520841)
  • Removed the custom Settings.describe() and Settings.__repr__() methods;
    Pydantic's existing model representation is used instead. (2f6d2c2)
  • Removed the unused table_path() helper and its io re-export. Writers and
    their WriteSpec entries continue to own canonical table subdirectories.
    (093549c)
  • Removed the string-returning output_root() helper. ETL notebooks and the ETL
    prompt now use the absolute Path from get_settings().output_root and join
    subpaths with /. Associated helper tests and exports were removed.
    (537dcb2)

Batch normalization and write validation

  • Made write_models() the only input-normalization boundary. It accepts one
    Pydantic model or an iterable, materializes iterables once, rejects empty or
    non-Pydantic inputs, and requires every item to have the same exact concrete
    model type. (57b9c4c)
  • Changed validate_for_write() to accept only a non-empty
    Sequence[BaseModel], validate every member against the exact
    spec.model_cls, and return a new list containing the original instances.
    It no longer normalizes single models or generators. (57b9c4c)
  • Made the supplied WriteSpec authoritative for direct validation.
    strict_model_for(spec) no longer consults the global registry, and its
    bounded cache is keyed by the model class and sorted required-field policy.
    write_models() still obtains its spec from the registry. (57b9c4c)
  • Narrowed WriteSpec.model_cls, get_spec(), and WRITABLE_CLASSES to
    Pydantic model classes and removed the redundant validation forwarding hook.
    (57b9c4c)
  • Added regression coverage for supported writer input shapes, invalid and
    heterogeneous batches, later-member failures, exact-type validation, custom
    spec authority, cache isolation, and preservation of input object identity.
    (57b9c4c)
  • Updated the unreleased changelog entry to describe the final
    strict_model_for(spec) and validate_for_write(models, spec) contracts.
    (57b9c4c)
  • Replaced truthiness-based empty checks with explicit len(...) == 0 checks at
    the reviewed sequence and scope boundaries. (ac3b830, f2fd6ea)

Projection writer typing

  • Replaced Any with ProjectionMeasurementMatrix for projection metadata and
    numpy.typing.ArrayLike for matrix inputs in both
    write_projection_matrix() and populate_region_coverage().
    (f18e278)
  • Kept the existing numpy.asarray conversion and runtime shape checks, and
    added a nested-list test to confirm that non-NumPy array-like inputs remain
    supported without mutating the original model. (f18e278)

Contract and test documentation

  • Expanded write-path docstrings to describe parameter roles, return values,
    errors, invariants, and IO side effects. This includes the reviewed contracts
    for validate_for_write(), _dispatch_overwrite_scoped(),
    _dispatch_append_new_by_id(), and _resolve_output_root(). (a04579e)
  • Corrected the append_new_dataitems() documentation to limit duplicate
    prevention to sequential calls where the existing Delta table can be read;
    it does not claim concurrency protection or idempotency after read failures.
    (a04579e)
  • Removed the stale hardcoded writable-class list from write_models()
    documentation in favor of runtime discovery through WRITABLE_CLASSES.
    (ac3b830)
  • Added concise behavioral docstrings across 13 test modules, including the
    requested config and Parquet-loader tests. (429c9c8)

Supporting repository changes

  • Added review-planning documents and implementation plans. These record some of the
    review topics. (d39a31a, 0848c73,
    a04579e)
  • Added a repository-local docstring writing and auditing skill.
    (1dc361a)
  • Added a future configuration architecture document. That commit changed only
    planning documentation; it did not untrack ccc_config.yaml or alter runtime
    configuration behavior. (f4be99f)
  • Added the Code Ocean secret declaration used for V1DD authentication.
    (8563537)

@reneyagmur
reneyagmur requested a review from danielsf August 18, 2026 16:34
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.

3 participants