Skip to content

Commit d2b8e33

Browse files
ghansemwojtyczka
andauthored
Fix app installation for Windows (#1327)
## Changes <!-- Summary of your changes that are easy to understand. Add screenshots when necessary --> This PR fixes the DQX Studio installation script (`build_app.py`) for Windows machines. We add the `.cmd` suffix to node binaries after detecting the OS: ``` def _node_bin(name: str) -> str: """Return the path to a ``node_modules/.bin`` executable for this platform.""" suffix = ".cmd" if os.name == "nt" else "" return str(NODE_BIN / f"{name}{suffix}") ``` Minor fix: Add job sweep to remove orphaned jobs in CI ### Linked issues <!-- DOC: Link issue with a keyword: close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved. See https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword --> Resolves #1326 ### Tests <!-- How is this tested? Please see the checklist below and also describe any other relevant tests --> - [x] manually tested - [x] added unit tests - [ ] added integration tests - [ ] added end-to-end tests - [ ] added performance tests ### Documentation and Demos <!-- Any user facing changes require documentation and demos update --> - [ ] added/updated demos - [ ] added/updated docs - [ ] added/updated agent skills --------- Co-authored-by: Marcin Wojtyczka <marcin.wojtyczka@databricks.com>
1 parent a351cac commit d2b8e33

4 files changed

Lines changed: 125 additions & 3 deletions

File tree

app/scripts/build_app.py

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
from __future__ import annotations
3737

3838
import json
39+
import os
3940
import shutil
4041
import subprocess
4142
import tomllib
@@ -69,6 +70,12 @@ def _run(cmd: list[str]) -> None:
6970
subprocess.run(cmd, cwd=APP_DIR, check=True) # noqa: S603
7071

7172

73+
def _node_bin(name: str) -> str:
74+
"""Return the path to a ``node_modules/.bin`` executable for this platform."""
75+
suffix = ".cmd" if os.name == "nt" else ""
76+
return str(NODE_BIN / f"{name}{suffix}")
77+
78+
7279
def _load_pyproject() -> dict:
7380
with PYPROJECT.open("rb") as f:
7481
return tomllib.load(f)
@@ -160,7 +167,7 @@ def _run_orval() -> None:
160167
locally-installed binary directly so the pinned ``orval`` is used
161168
regardless of which package manager is on PATH.
162169
"""
163-
_run([str(NODE_BIN / "orval")])
170+
_run([_node_bin("orval")])
164171

165172

166173
def _run_vite_build() -> None:
@@ -169,7 +176,7 @@ def _run_vite_build() -> None:
169176
Vite reads ``vite.config.ts``, which sets ``build.outDir`` to the
170177
package's ``__dist__`` folder so it ships inside the package source.
171178
"""
172-
_run([str(NODE_BIN / "vite"), "build"])
179+
_run([_node_bin("vite"), "build"])
173180

174181

175182
def _assemble_deploy_tree() -> None:

app/tests/test_build_app.py

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
"""Tests for ``app/scripts/build_app.py``."""
2+
3+
from __future__ import annotations
4+
5+
import importlib.util
6+
from pathlib import Path
7+
8+
import pytest
9+
10+
BUILD_APP_PATH = Path(__file__).resolve().parent.parent / "scripts" / "build_app.py"
11+
12+
13+
@pytest.mark.parametrize(
14+
("runner", "tool", "extra_args"),
15+
[
16+
("_run_orval", "orval", []),
17+
("_run_vite_build", "vite", ["build"]),
18+
],
19+
)
20+
@pytest.mark.parametrize(
21+
("os_name", "expected_suffix"),
22+
[("posix", ""), ("nt", ".cmd")],
23+
)
24+
def test_node_runners_pick_platform_shim(monkeypatch, runner, tool, extra_args, os_name, expected_suffix):
25+
captured = []
26+
build_app = _load_build_app()
27+
28+
def _fake_run(cmd, **_kwargs):
29+
captured.append(cmd)
30+
31+
monkeypatch.setattr(build_app.os, "name", os_name)
32+
monkeypatch.setattr(build_app.subprocess, "run", _fake_run)
33+
34+
getattr(build_app, runner)()
35+
36+
cmd = captured[0]
37+
assert cmd[0] == str(build_app.NODE_BIN / f"{tool}{expected_suffix}")
38+
assert cmd[1:] == extra_args
39+
40+
41+
def _load_build_app():
42+
spec = importlib.util.spec_from_file_location("build_app", BUILD_APP_PATH)
43+
module = importlib.util.module_from_spec(spec)
44+
spec.loader.exec_module(module)
45+
return module

src/databricks/labs/dqx/installer/install.py

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -346,13 +346,19 @@ def uninstall(self):
346346
return
347347

348348
logger.info(f"Deleting DQX v{self._product_info.version()} from {self._ws.config.host}")
349+
350+
# Remove workflow jobs first, independently of the install folder. Jobs and the install folder
351+
# are separate resources: if the folder is already gone (partial/failed install, or a prior
352+
# cleanup step), gating job removal behind files() would orphan the jobs and leak them toward
353+
# the workspace job limit.
354+
self._workflow_installer.remove_jobs()
355+
349356
try:
350357
self._installation.files() # this also deletes the dashboard
351358
except NotFound:
352359
logger.error(f"Check if {self._installation.install_folder()} is present")
353360
return
354361

355-
self._workflow_installer.remove_jobs()
356362
default_run_config = self._config.get_run_config()
357363
self._warehouse_configurator.remove(default_run_config.warehouse_id)
358364
self._installation.remove()

tests/integration/test_installation.py

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,9 @@
11
import logging
2+
import os
3+
import re
4+
from collections.abc import Callable
5+
from datetime import datetime, timedelta, timezone
6+
from functools import partial
27
from unittest import skip
38
from unittest.mock import patch, create_autospec
49
import pytest
@@ -609,3 +614,62 @@ def test_install_config_rejects_no_output_and_no_quarantine(ws):
609614
match="At least one of an output table or a quarantine table",
610615
):
611616
installer.configure()
617+
618+
619+
# DQX installs its workflows as jobs named "[<INSTALL_FOLDER>] <workflow>" (see mixins._get_name);
620+
# the workflow suffix is one of the deployed steps below.
621+
_DQX_JOB_NAME_PATTERN = re.compile(r"^\[.+\] (profiler|quality-checker|e2e|anomaly-trainer)$")
622+
623+
624+
def _safe_delete_job(delete: Callable[[], object], name: str) -> None:
625+
"""Best-effort delete: a single failure is logged and never blocks the rest of the sweep."""
626+
try:
627+
delete()
628+
except NotFound:
629+
pass
630+
except Exception as e: # noqa: BLE001 — best-effort sweep must not block the test run
631+
logger.warning(f"Failed to delete orphaned DQX job '{name}': {e}")
632+
633+
634+
def _remove_orphaned_jobs(ws) -> None:
635+
"""Delete orphaned DQX workflow jobs (profiler, quality-checker, e2e, anomaly-trainer) left behind
636+
by cancelled CI runs.
637+
638+
Orphaned jobs accumulate when a github action is cancelled and the ``installation_ctx`` fixture
639+
teardown (which calls ``uninstall()`` to remove the deployed jobs) does not run. Jobs count toward
640+
the per-workspace job limit, so they are swept explicitly here, mirroring the orphaned-Lakebase
641+
sweep.
642+
643+
Runs only in CI. Jobs younger than the 2h grace period are skipped so a concurrent run's freshly
644+
deployed jobs (and the current run's own jobs) are never deleted; a real long-lived install is
645+
protected because the sweep only runs in the ephemeral CI test workspace.
646+
"""
647+
run_id = os.getenv("GITHUB_RUN_ID")
648+
if not run_id:
649+
return # only applicable when run in CI
650+
651+
grace_period = datetime.now(timezone.utc) - timedelta(hours=2) # aligned with tests timeout
652+
653+
matched = attempted = 0
654+
for job in ws.jobs.list():
655+
name = (job.settings.name if job.settings else None) or ""
656+
if not _DQX_JOB_NAME_PATTERN.match(name):
657+
continue
658+
matched += 1
659+
created = datetime.fromtimestamp(job.created_time / 1000, tz=timezone.utc) if job.created_time else None
660+
if created is not None and created >= grace_period:
661+
continue # too young — may belong to a concurrent or the current run
662+
_safe_delete_job(partial(ws.jobs.delete, job_id=job.job_id), name)
663+
attempted += 1
664+
logger.info(f"DQX job sweep: matched_naming={matched} delete_attempts={attempted}")
665+
666+
667+
def test_remove_orphaned_jobs(ws):
668+
"""Maintenance sweep: remove orphaned DQX workflow jobs left by cancelled CI runs.
669+
670+
Mirrors the orphaned-Lakebase-resource sweep. Deployed workflow jobs (profiler, quality-checker,
671+
e2e, anomaly-trainer) count toward the per-workspace job limit and are only cleaned by the
672+
installation fixture teardown, so a cancelled run leaks them. Runs only in CI; jobs younger than
673+
the 2h grace period are skipped.
674+
"""
675+
_remove_orphaned_jobs(ws)

0 commit comments

Comments
 (0)