Skip to content

fix(#1493): build self.upstream lazily; stop reloading deps per populate key#1499

Merged
dimitri-yatsenko merged 1 commit into
masterfrom
fix/1493-lazy-upstream
Jul 16, 2026
Merged

fix(#1493): build self.upstream lazily; stop reloading deps per populate key#1499
dimitri-yatsenko merged 1 commit into
masterfrom
fix/1493-lazy-upstream

Conversation

@dimitri-yatsenko

Copy link
Copy Markdown
Member

Fixes the per-make() cost profiled in #1493 (~53 ms and ~59 information_schema round-trips per populate key, even when make() never reads self.upstream).

1. Lazy self.upstream (autopopulate.py): _populate1 records the make() key (_upstream_key) instead of eagerly building Diagram.trace(). The trace is built on first self.upstream access and memoized for the call, so make() calls that never touch upstream pay nothing. Both _upstream and _upstream_key are cleared in finally.

2. Idempotent dependency loading (dependencies.py): track _loaded_schemas; load_all_upstream/load_all_downstream skip the full information_schema rebuild when the graph already contains every needed schema — so repeated Diagram.trace() calls within one populate() don't reload the dependency tree per key.

Tests: three self.upstream tests pinned the old eager internal (self._upstream non-None throughout make); updated to the lazy contract. One now asserts self._upstream stays None when make() never reads upstream (proving laziness); the tripartite test captures id(self.upstream) (the property) to verify one shared object across phases. Locally: 6/6 upstream + 76 trace/cascade/delete/erd tests pass on MySQL and PostgreSQL.

Closes #1493.

…ate key

Two changes that eliminate the ~53 ms / ~59 information_schema round-trips per populate key introduced by the self.upstream feature:

1. Lazy self.upstream (autopopulate.py): _populate1 records the make() key instead of eagerly building Diagram.trace(); the trace is constructed on first self.upstream access and memoized for the call. make() calls that never read upstream now pay nothing. New _upstream_key marks 'inside make()'; both it and _upstream are cleared in finally.

2. Idempotent dependency loading (dependencies.py): track _loaded_schemas; load_all_upstream/load_all_downstream skip the full information_schema rebuild when the graph already contains all needed schemas, so repeated Diagram.trace() calls within one populate() don't reload the dependency tree per key.

Tests: updated three self.upstream tests that pinned the old eager internal (self._upstream non-None throughout make) to the lazy contract — one now asserts self._upstream stays None when make() never reads upstream (proving laziness); the tripartite test captures id(self.upstream) (the property) to verify the same object is shared across phases. 6/6 upstream + 76 trace/cascade/delete/erd tests pass.

Closes #1493.
@dimitri-yatsenko dimitri-yatsenko added this to the v2.3.1 milestone Jul 16, 2026

@MilagrosMarin MilagrosMarin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @dimitri-yatsenko — clean two-part fix for #1493. Verified end-to-end:

1. Lazy self.upstream (autopopulate.py). _populate1 now sets _upstream_key = dict(key) and _upstream = None before invoking make(); the property builds Diagram.trace(self & self._upstream_key) on first access and memoizes. The "outside make()" gate is correctly switched from _upstream is None to _upstream_key is None — otherwise the very first access inside make() would raise. finally clears both. Tripartite path (fetch/compute/insert with the generator restart at line 720) shares the same _upstream slot across phases, so id(self.upstream) stays stable — the updated test assertion holds.

2. Idempotent dep loading (dependencies.py). _loaded_schemas tracks what's in the graph; load_all_upstream/load_all_downstream short-circuit when _loaded and known_schemas ⊆ _loaded_schemas. Correctness cases hold: first call (unloaded) → reload; repeated call with same schemas → skip; discovery finds a new schema → superset check fails → reload; clear() resets both fields. load() continues to unconditionally repopulate _loaded_schemas, so direct load(force=True, ...) callers still get a fresh graph.

Test updates are on-point. test_upstream_cleared_after_make now asserts self._upstream is None inside the make() body that doesn't read upstream — that's the specific regression guard for the laziness claim, and it would fail against the old eager code. The tripartite test's switch from id(self._upstream) to id(self.upstream) correctly captures the property (which triggers the memoized build) rather than the raw internal attribute.

Two small observations, neither blocking:

  • No test for the dep-loading idempotence itself. A query-count assertion (information_schema round-trips = N on first Diagram.trace(), 0 on repeats) would concretely pin the perf fix — otherwise a future refactor could break the short-circuit silently. The existing tests cover correctness; this would cover the perf guarantee the PR's motivation rests on.
  • Private-attribute contract shifted. self._upstream is None no longer discriminates "outside make()" — self._upstream_key does. Since these are underscore-prefixed with no stability promise, this is fine; the test file itself updated to the new discriminator. Just worth being aware if any external code (plugin, extension, or user script) introspected _upstream for that signal.

Approving.

@dimitri-yatsenko
dimitri-yatsenko merged commit 29ce695 into master Jul 16, 2026
9 checks passed
@dimitri-yatsenko
dimitri-yatsenko deleted the fix/1493-lazy-upstream branch July 16, 2026 22:38
@dimitri-yatsenko dimitri-yatsenko added the enhancement Indicates new improvements label Jul 16, 2026
MilagrosMarin added a commit that referenced this pull request Jul 20, 2026
…lass Attributes

Two post-#1499 docstring/comment updates on autopopulate.py:

1. The finally-block comment in _populate_one attributed the 'raises a
   clear error' behavior to `self._upstream = None`, but post-#1499
   that line only invalidates the memoized Diagram. The actual guard
   is `self._upstream_key = None` (via the `upstream` property at
   :135-140). Reworded to attribute each line to its role.

2. AutoPopulate class docstring's Attributes section listed only
   `key_source` and `jobs`, omitting `upstream` - which after #1473 is
   a public property and the recommended read surface inside make().
   Added it alongside the others.

Docs-only.
dimitri-yatsenko pushed a commit that referenced this pull request Jul 20, 2026
…gs from ambient env (#1513)

* tests: add coverage for deferred audit findings + isolate test_settings from ambient env

Two test-suite hardening changes, all in tests/, no source-code changes:

## 1. Coverage additions for 3 audit-deferred findings (6 test functions)

- tests/integration/test_dependencies.py TestLoadAllShortCircuit (2 tests)
  Guards the ~53ms/populate-key perf fix from PR #1499 by spying on
  Dependencies.load during a repeat load_all_upstream/load_all_downstream
  call. Verified: gating the early-return at dependencies.py:266-267,
  :300-301 with 'if False and ...' makes both tests fail; restored.

- tests/integration/test_gc.py TestDeleteSchemaPathPruning (2 tests)
  Covers the parent-directory pruning loop in gc.py delete_schema_path.
  Inserts two rows in separate pk-dirs, deletes one, runs collect(),
  asserts the orphan's pk-dir is pruned, the sibling's is preserved,
  and the store root is untouched. Verified: removing the pruning loop
  at gc.py:293-302 makes test_pruning_removes_empty_pk_directory fail;
  restored.

- tests/integration/test_object.py TestStoragePathGeneration (2 tests)
  Covers build_object_path with schema_prefix='' (the documented
  pre-2.3.1 legacy layout — not previously tested). Asserts the path
  is {schema}/{table}/pk=v/field_token.dat (no leading empty segment).
  Verified: removing the 'if prefix:' guard at storage.py:267-269 makes
  both tests fail; restored.

## 2. Env-isolation fixture for tests/unit/test_settings.py

Four tests in TestStoreEnv and TestBackendConfiguration read DJ_HOST/
DJ_USER/DJ_PASS/DJ_PORT/DJ_BACKEND via pydantic-settings and fail if any
of those vars is exported in the developer's shell — even when the test's
own contract is 'ignore config file' or 'auto-detect port'. CI runs in a
clean env so this only bites devs who source a local .env, but the
tests' contract shouldn't depend on ambient env cleanliness.

Adds a class-level @pytest.fixture(autouse=True) that monkeypatch.delenv's
the 5 DJ_* vars at the top of each test in both classes. Individual tests
that need to set DJ_* explicitly still work — the fixture runs first, so
their setenv wins.

Verified: pytest tests/unit/test_settings.py::TestStoreEnv   tests/unit/test_settings.py::TestBackendConfiguration → 19/19 pass
locally with the fixture.

* style: apply ruff-format (collapse multi-line asserts where they fit)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Indicates new improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

populate: self.upstream (Diagram.trace) built unconditionally per make() — ~53 ms + 59 information_schema queries/key

2 participants