Detect dbutils.notebook.run #1284

ericvergnaud · 2024-04-05T14:57:17Z

Changes

detect dependencies on notebooks called via dbutils.notebook.run

Linked issues

Resolves #1200

add sample with RUN cell fix issue where non-PI comments preceding language PI would prevent language PI detection

* main: remove `isort` (databrickslabs#1280) Addressed Issue with Disabled Feature in certain regions (databrickslabs#1275) Improve documentation (databrickslabs#1162) Add roadmap workflows and tasks to Table Migration Workflow document (databrickslabs#1274) Fix integration test with new DeployedWorkflows (databrickslabs#1250) Document troubleshooting guide (databrickslabs#1226) Split `DeployedWorkflows` out of `WorkflowsDeployment` (databrickslabs#1248) Inject `_TASKS` via constructor to `WorkflowsDeployment` instead of a global variable (databrickslabs#1247) Decouple `InstallState` from `WorkspaceDeployment` constructor Add document for table migration workflow (databrickslabs#1229) Decouple `InstallState` from `WorkflowsDeployment` constructor (databrickslabs#1246)

* main: Build notebook dependency graph for `%run` cells (databrickslabs#1279) # Conflicts: # src/databricks/labs/ucx/source_code/notebook.py # tests/unit/source_code/test_notebook.py

src/databricks/labs/ucx/source_code/astlinter.py

src/databricks/labs/ucx/source_code/notebook.py

Co-authored-by: Cor <[email protected]>

codecov · 2024-04-08T19:29:48Z

Codecov Report

Attention: Patch coverage is 88.23529% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 89.23%. Comparing base (b2a11cf) to head (e1fcb4f).
Report is 35 commits behind head on main.

❗ Current head e1fcb4f differs from pull request most recent head 2224c3f. Consider uploading reports for the commit 2224c3f to get more accurate results

Files	Patch %	Lines
...c/databricks/labs/ucx/source_code/python_linter.py	81.15%	8 Missing and 5 partials ⚠️
src/databricks/labs/ucx/source_code/notebook.py	94.04%	2 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1284      +/-   ##
==========================================
- Coverage   90.02%   89.23%   -0.79%     
==========================================
  Files          62       70       +8     
  Lines        7430     8242     +812     
  Branches     1335     1454     +119     
==========================================
+ Hits         6689     7355     +666     
- Misses        470      589     +119     
- Partials      271      298      +27

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/databricks/labs/ucx/source_code/astlinter.py

nfx · 2024-04-08T19:29:05Z

src/databricks/labs/ucx/source_code/astlinter.py

+class MatchingVisitor(ast.NodeVisitor):
+
+    def __init__(self, node_type: type, match_nodes: list[tuple[str, type]]):
+        self.matched_nodes: list[ast.AST] = []


if something is public, let's expose this as a method - would be easier to refactor.

nfx · 2024-04-08T19:31:03Z

src/databricks/labs/ucx/source_code/notebook.py

+class PythonLinter(ASTLinter, Linter):
+    def lint(self, code: str) -> Iterable[Advice]:
+        self.parse(code)
+        nodes = self.locate(ast.Call, [("run", ast.Attribute), ("notebook", ast.Attribute), ("dbutils", ast.Name)])


what if the code is self._dbutils.notebook.run(...)?

I agree it's theoretically possible to do that but I can't think of any benefit for users to keep a local private copy of a public API ?

To your point, we can't address this edge cases for now. Tbh, I can think of thousands of them, such as:

run_notebook = dbutils.notebook.run .../... run_notebook('some notebook')

I have created ticket #1334 for that

nfx · 2024-04-08T19:33:53Z

src/databricks/labs/ucx/source_code/astlinter.py

+    def __init__(self):
+        self._module: ast.Module | None = None
+
+    def parse(self, code: str):
+        self._module = ast.parse(code)


Suggested change

def __init__(self):

self._module: ast.Module | None = None

def parse(self, code: str):

self._module = ast.parse(code)

def __init__(self, code: str):

self._module = ast.parse(code)

so that we either fail initialising or have a valid state.

I respectfully disagree. A constructor should not perform processing, especially not when that processing may fail under uncontrolled conditions (source code received from outside)

Factory is just fine

actually the reason for this is that ASTLinter is a base class for PythonLinter, which also needs to follow Linter conventions... changing that now

nfx · 2024-04-08T19:35:59Z

src/databricks/labs/ucx/source_code/notebook.py

@@ -94,7 +150,8 @@ def is_runnable(self) -> bool:
            statements = parse_sql(self._original_code)
            return len(statements) > 0
        except SQLParseError:
-            return False
+            sqlglot_logger.warning(f"Failed to parse SQL using 'sqlglot': {self._original_code}")


let's also log the sqlglot error, so that we can create issues over there.

nfx · 2024-04-08T19:38:35Z

src/databricks/labs/ucx/source_code/notebook.py

-        self._new_cell = args[3]
+        # PI stands for Processing Instruction
+        # pylint: disable=invalid-name
+        self._requires_isolated_PI = args[3]


Suggested change

self._requires_isolated_PI = args[3]

self._requires_isolated_processing_instruction = args[3]

:)

fixed the warning, but I guess people looking into this code would be informed enough by the comment ?

nfx · 2024-04-08T19:39:38Z

src/databricks/labs/ucx/source_code/notebook.py

+                if cell_language.requires_isolated_pi:
+                    if line.startswith(LANGUAGE_PREFIX):


Suggested change

if cell_language.requires_isolated_pi:

if line.startswith(LANGUAGE_PREFIX):

if cell_language.requires_isolated_pi and line.startswith(LANGUAGE_PREFIX):

nfx · 2024-04-08T19:39:54Z

src/databricks/labs/ucx/source_code/notebook.py

+    def _make_runnable(self, lines: list[str], cell_language: CellLanguage):
+        prefix = f"{self.comment_prefix} {MAGIC_PREFIX} "
+        prefix_len = len(prefix)
+        # pylint: disable=too-many-nested-blocks


Suggested change

# pylint: disable=too-many-nested-blocks

this one can be avoided

nfx · 2024-04-08T19:41:08Z

src/databricks/labs/ucx/source_code/notebook.py

@@ -225,6 +292,41 @@ def make_cell(lines_: list[str]):

        return cells

+    def _make_runnable(self, lines: list[str], cell_language: CellLanguage):


Suggested change

def _make_runnable(self, lines: list[str], cell_language: CellLanguage):

def _unwrap_magic(self, lines: list[str], cell_language: CellLanguage):

i think it's a better name for this

nfx · 2024-04-08T19:41:28Z

src/databricks/labs/ucx/source_code/notebook.py

+                line = f"{cell_language.comment_prefix} {COMMENT_PI}{line}"
+                lines[i] = line
+
+    def make_unrunnable(self, code: str, cell_language: CellLanguage) -> str:


Suggested change

def make_unrunnable(self, code: str, cell_language: CellLanguage) -> str:

def as_magic(self, code: str, cell_language: CellLanguage) -> str:

might be a better name

nfx · 2024-04-09T09:08:22Z

src/databricks/labs/ucx/source_code/astlinter.py

+    def matched_nodes(self):
+        return self._matched_nodes
+
+    # pylint: disable=invalid-name


Suggested change

# pylint: disable=invalid-name

# visit_Call is the invalid naming convention, but it is required for NodeVisitor

# pylint: disable=invalid-name

nfx · 2024-04-09T09:19:00Z

src/databricks/labs/ucx/source_code/notebook.py

@@ -38,6 +51,43 @@ def build_dependency_graph(self, parent: DependencyGraph):
        raise NotImplementedError()


+class PythonLinter(Linter):


move this to astlinter.py ( -> python_analysis.py)

nfx · 2024-04-09T09:20:52Z

src/databricks/labs/ucx/source_code/notebook.py

+        path = cls.get_dbutils_notebook_run_path_arg(node)
+        if isinstance(path, ast.Constant):
+            return Advisory(
+                'notebook-auto-migrate',


Suggested change

'notebook-auto-migrate',

'dbutils-notebook-run-literal',

nfx · 2024-04-09T09:21:16Z

src/databricks/labs/ucx/source_code/notebook.py

+                node.end_col_offset or 0,
+            )
+        return Advisory(
+            'notebook-manual-migrate',


Suggested change

'notebook-manual-migrate',

'dbutils-notebook-run-dynamic',

* main: Adding CSV, JSON and include path in mounts (databrickslabs#1329) Add missing step sync-workspace-info (databrickslabs#1330) disable annotation-unchecked mypy warning (databrickslabs#1331) Use service factory to resolve object dependencies (databrickslabs#1209)

nfx

lgtm

* main: Integrate detection of notebook dependencies (databrickslabs#1338) # Conflicts: # src/databricks/labs/ucx/source_code/notebook.py

nfx · 2024-04-10T12:42:05Z

merged

ericvergnaud added 23 commits April 4, 2024 12:31

add test util for loading local source

e47b965

add Notebook samples

a0c1e16

rename sample to avoid fmt errors

a2808ce

allow loading local notebook samples

4066042

reads cells from notebook samples

12e075c

fix pylint issues

ee88135

uncomment

c428e18

ensure notebook is rebuilt properly after migration

c7e0065

builds leaf dependency graph

d0ce635

add support for RUN cells

38e18cc

add sample with RUN cell fix issue where non-PI comments preceding language PI would prevent language PI detection

add samples for dependency graph

c3c160e

builds dependency graph of depth 1 in %run cells

9fd6a11

builds a multi-level dependency graph

8e36094

caters for duplicates

cece88d

support cyclical dependencies

3a39712

fix format

63b1d82

fix failing test - source_code dir contains non-python files

946d998

fix pylint issue

da61c2b

fix failing tests

b43dd5f

rename notebooks to NotebookMigrator

81fffd3

align naming

6cdcfd4

Merge branch 'main' into detect-dbutils.notebook.run

80c07a5

* main: Build notebook dependency graph for `%run` cells (databrickslabs#1279) # Conflicts: # src/databricks/labs/ucx/source_code/notebook.py # tests/unit/source_code/test_notebook.py

ericvergnaud requested review from a team and larsgeorge-db April 5, 2024 14:57

ericvergnaud marked this pull request as draft April 5, 2024 14:57

ericvergnaud added 4 commits April 5, 2024 19:24

drop renamed samples

59d1ece

make cell code runnable

fa785fe

add sample

036d5fc

convert to python 3

8796eec

JCZuurmond reviewed Apr 8, 2024

View reviewed changes

src/databricks/labs/ucx/source_code/astlinter.py Outdated Show resolved Hide resolved

src/databricks/labs/ucx/source_code/notebook.py Outdated Show resolved Hide resolved

ericvergnaud and others added 2 commits April 8, 2024 13:56

Update src/databricks/labs/ucx/source_code/astlinter.py

cd58d7a

Co-authored-by: Cor <[email protected]>

Update src/databricks/labs/ucx/source_code/notebook.py

f0770af

Co-authored-by: Cor <[email protected]>

ericvergnaud marked this pull request as ready for review April 8, 2024 12:01

ericvergnaud changed the title ~~[WIP] Detect dbutils.notebook.run~~ Detect dbutils.notebook.run Apr 8, 2024

ericvergnaud added 3 commits April 8, 2024 15:37

add notebook samples

12c8e30

refactor for supporting both linting and dependency detection

33495a1

raise distinct Advisory for manual vs automated migration

c005c46

ericvergnaud had a problem deploying to account-admin April 8, 2024 19:26 — with GitHub Actions Failure

nfx requested changes Apr 8, 2024

View reviewed changes

address code review comments

ea181b4

ericvergnaud had a problem deploying to account-admin April 9, 2024 07:49 — with GitHub Actions Failure

change model to avoid clashing ownership patterns

1ddd452

ericvergnaud requested a review from nfx April 9, 2024 07:55

ericvergnaud had a problem deploying to account-admin April 9, 2024 09:03 — with GitHub Actions Failure

nfx requested changes Apr 9, 2024

View reviewed changes

ericvergnaud added 2 commits April 9, 2024 12:00

move PythonLinter to python_linter file renamed from astlinter

fa6a8cf

address code review comments

967f956

ericvergnaud requested a review from nfx April 9, 2024 10:10

nfx approved these changes Apr 9, 2024

View reviewed changes

ericvergnaud had a problem deploying to account-admin April 9, 2024 12:47 — with GitHub Actions Failure

nfx added the ready to merge label Apr 9, 2024

Merge branch 'main' into detect-dbutils.notebook.run

2224c3f

* main: Integrate detection of notebook dependencies (databrickslabs#1338) # Conflicts: # src/databricks/labs/ucx/source_code/notebook.py

nfx closed this Apr 10, 2024

ericvergnaud had a problem deploying to account-admin April 10, 2024 12:42 — with GitHub Actions Failure

ericvergnaud deleted the detect-dbutils.notebook.run branch April 12, 2024 10:36

nfx mentioned this pull request Apr 22, 2024

[FEATURE]: Detect notebook include graph by analysing dbutils.notebook.run(...) calls #1200

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect dbutils.notebook.run #1284

Detect dbutils.notebook.run #1284

ericvergnaud commented Apr 5, 2024 •

edited by nfx

Loading

codecov bot commented Apr 8, 2024 •

edited

Loading

nfx Apr 8, 2024

ericvergnaud Apr 9, 2024

nfx Apr 8, 2024

ericvergnaud Apr 9, 2024

nfx Apr 8, 2024

ericvergnaud Apr 9, 2024

nfx Apr 9, 2024

ericvergnaud Apr 9, 2024

ericvergnaud Apr 9, 2024

nfx Apr 8, 2024

ericvergnaud Apr 9, 2024

nfx Apr 8, 2024

ericvergnaud Apr 9, 2024

nfx Apr 8, 2024

nfx Apr 8, 2024

ericvergnaud Apr 9, 2024

nfx Apr 8, 2024

ericvergnaud Apr 9, 2024

nfx Apr 8, 2024

ericvergnaud Apr 9, 2024

nfx Apr 9, 2024

ericvergnaud Apr 9, 2024

nfx Apr 9, 2024

ericvergnaud Apr 9, 2024

nfx Apr 9, 2024

ericvergnaud Apr 9, 2024

nfx Apr 9, 2024

ericvergnaud Apr 9, 2024

nfx left a comment

nfx commented Apr 10, 2024

	self._requires_isolated_PI = args[3]
	self._requires_isolated_processing_instruction = args[3]

		if cell_language.requires_isolated_pi:
		if line.startswith(LANGUAGE_PREFIX):

	if cell_language.requires_isolated_pi:
	if line.startswith(LANGUAGE_PREFIX):
	if cell_language.requires_isolated_pi and line.startswith(LANGUAGE_PREFIX):

		@@ -225,6 +292,41 @@ def make_cell(lines_: list[str]):

		return cells

		def _make_runnable(self, lines: list[str], cell_language: CellLanguage):

	def _make_runnable(self, lines: list[str], cell_language: CellLanguage):
	def _unwrap_magic(self, lines: list[str], cell_language: CellLanguage):

	def make_unrunnable(self, code: str, cell_language: CellLanguage) -> str:
	def as_magic(self, code: str, cell_language: CellLanguage) -> str:

	# pylint: disable=invalid-name
	# visit_Call is the invalid naming convention, but it is required for NodeVisitor
	# pylint: disable=invalid-name

		@@ -38,6 +51,43 @@ def build_dependency_graph(self, parent: DependencyGraph):
		raise NotImplementedError()


		class PythonLinter(Linter):

Detect dbutils.notebook.run #1284

Detect dbutils.notebook.run #1284

Conversation

ericvergnaud commented Apr 5, 2024 • edited by nfx Loading

Changes

Linked issues

codecov bot commented Apr 8, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfx left a comment

Choose a reason for hiding this comment

nfx commented Apr 10, 2024

ericvergnaud commented Apr 5, 2024 •

edited by nfx

Loading

codecov bot commented Apr 8, 2024 •

edited

Loading