Skip to content

perf: cache parsed manifest in APIDbtRunner #2196

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dtaniwaki wants to merge 2 commits into from
+96 −1
Closed

perf: cache parsed manifest in APIDbtRunner #2196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d6d6d06
perf: cache parsed manifest in APIDbtRunner
dtaniwaki Apr 22, 2026
ccb3fe8
test: add unit tests for APIDbtRunner manifest caching
dtaniwaki Apr 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion elementary/clients/dbt/api_dbt_runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ class APIDbtCommandResult(DbtCommandResult):


class APIDbtRunner(CommandLineDbtRunner):
def __init__(self, *args, **kwargs):
self._manifest = None
super().__init__(*args, **kwargs)

def _inner_run_command(
self,
dbt_command_args: List[str],
Expand All @@ -45,9 +49,11 @@ def collect_dbt_command_logs(event):
dbt_logs.append(event_dump)

with env_vars_context(self.env_vars):
dbt = dbtRunner(callbacks=[collect_dbt_command_logs])
dbt = dbtRunner(manifest=self._manifest, callbacks=[collect_dbt_command_logs])
with with_chdir(self.project_dir):
res: dbtRunnerResult = dbt.invoke(dbt_command_args)
if self._manifest is None and res.success:
self._manifest = dbt.manifest
output = "\n".join(dbt_logs) or None
# Surface the exception text so that transient-error detection in
# _inner_run_command_with_retries can match against it. The dbt
Expand Down
89 changes: 89 additions & 0 deletions tests/unit/clients/dbt_runner/test_api_dbt_runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
from contextlib import contextmanager
from unittest import mock

from dbt.cli.main import dbtRunnerResult

from elementary.clients.dbt.api_dbt_runner import APIDbtRunner


def _make_result(success=True, exception=None):
return dbtRunnerResult(
success=success,
result=None,
exception=exception,
)


def _make_runner():
runner = APIDbtRunner.__new__(APIDbtRunner)
runner._manifest = None
runner.project_dir = "/tmp/fake"
runner.env_vars = None
runner.raise_on_failure = False
return runner
Comment on lines +17 to +23
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 22, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Cover the real initializer and avoid the linted /tmp path.

_make_runner() is fine for isolating _inner_run_command, but it manually sets _manifest, so these tests would still pass if APIDbtRunner.__init__ stopped initializing _manifest before super().__init__(). Also, Line 20 is flagged by Ruff S108.

Proposed test/helper adjustment 
 def _make_runner():
     runner = APIDbtRunner.__new__(APIDbtRunner)
     runner._manifest = None
-    runner.project_dir = "/tmp/fake"
+    runner.project_dir = "fake_project_dir"
     runner.env_vars = None
     runner.raise_on_failure = False
     return runner
+
+
+def test_manifest_initialized_before_parent_init():
+    def assert_manifest_initialized(self, *args, **kwargs):
+        assert hasattr(self, "_manifest")
+        assert self._manifest is None
+
+    with mock.patch(
+        "elementary.clients.dbt.api_dbt_runner.CommandLineDbtRunner.__init__",
+        assert_manifest_initialized,
+    ):
+        APIDbtRunner(project_dir="fake_project_dir")
🧰 Tools
🪛 Ruff (0.15.10)

[error] 20-20: Probable insecure usage of temporary file or directory: "/tmp/fake"

(S108)

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tests/unit/clients/dbt_runner/test_api_dbt_runner.py` around lines 17 - 23,
Replace the helper that bypasses APIDbtRunner.__init__ with one that calls the
real initializer (so _manifest is set by the class) and avoid the hardcoded
"/tmp" by using a temporary directory fixture; e.g., change _make_runner to call
APIDbtRunner(...) with project_dir=str(tmp_path) (or a
tempfile.TemporaryDirectory context) and monkeypatch/stub any heavy external
behavior if needed instead of assigning runner._manifest manually, ensuring
tests exercise the actual __init__ behavior and remove the Ruff S108 `/tmp`
literal.



@contextmanager
def _noop_context(*args, **kwargs):
yield


_PATCH_CHDIR = mock.patch("elementary.clients.dbt.api_dbt_runner.with_chdir", _noop_context)
_PATCH_ENV = mock.patch("elementary.clients.dbt.api_dbt_runner.env_vars_context", _noop_context)


@_PATCH_ENV
@_PATCH_CHDIR
@mock.patch("elementary.clients.dbt.api_dbt_runner.dbtRunner")
def test_manifest_cached_after_first_success(mock_dbt_runner_cls):
fake_manifest = object()
mock_instance = mock.MagicMock()
mock_instance.invoke.return_value = _make_result(success=True)
mock_instance.manifest = fake_manifest
mock_dbt_runner_cls.return_value = mock_instance

runner = _make_runner()
runner._inner_run_command(["run-operation", "foo"], quiet=True, log_output=False, log_format="json")

assert runner._manifest is fake_manifest
mock_dbt_runner_cls.assert_called_once_with(manifest=None, callbacks=mock.ANY)


@_PATCH_ENV
@_PATCH_CHDIR
@mock.patch("elementary.clients.dbt.api_dbt_runner.dbtRunner")
def test_manifest_not_cached_on_failure(mock_dbt_runner_cls):
mock_instance = mock.MagicMock()
mock_instance.invoke.return_value = _make_result(success=False)
mock_instance.manifest = object()
mock_dbt_runner_cls.return_value = mock_instance

runner = _make_runner()
runner._inner_run_command(["run-operation", "foo"], quiet=True, log_output=False, log_format="json")

assert runner._manifest is None


@_PATCH_ENV
@_PATCH_CHDIR
@mock.patch("elementary.clients.dbt.api_dbt_runner.dbtRunner")
def test_cached_manifest_reused_on_subsequent_calls(mock_dbt_runner_cls):
fake_manifest = object()
mock_instance = mock.MagicMock()
mock_instance.invoke.return_value = _make_result(success=True)
mock_instance.manifest = fake_manifest
mock_dbt_runner_cls.return_value = mock_instance

runner = _make_runner()

runner._inner_run_command(["run-operation", "foo"], quiet=True, log_output=False, log_format="json")
assert runner._manifest is fake_manifest

new_manifest = object()
mock_instance.manifest = new_manifest
mock_dbt_runner_cls.reset_mock()

runner._inner_run_command(["run-operation", "bar"], quiet=True, log_output=False, log_format="json")

mock_dbt_runner_cls.assert_called_once_with(manifest=fake_manifest, callbacks=mock.ANY)
assert runner._manifest is fake_manifest
Loading