Infer values from child notebook loaded via dbutils.notebook.run() call #2083

ericvergnaud · 2024-07-04T15:34:42Z

Changes

When linting notebooks, use AST from child notebooks loaded via dbutils.notebook.run() to infer values

Linked issues

Progresses #1205
Progresses #1901

Functionality

None

Tests

added unit tests

marginally improves not computed (-9/744) when running make solacc

# Conflicts: # src/databricks/labs/ucx/source_code/linters/context.py

…rom-child-notebook-in-run-cell

nfx

pre-process a collection of cells into a single tree and revert any refactoring that makes this PR hard to review.

nfx · 2024-07-05T19:30:47Z

src/databricks/labs/ucx/source_code/linters/imports.py

-    @property
-    def is_append(self):
-        return self._is_append
+    def mutate_path_lookup(self, path_lookup: PathLookup):


Suggested change

def mutate_path_lookup(self, path_lookup: PathLookup):

def apply_to(self, path_lookup: PathLookup):

nfx · 2024-07-05T19:33:18Z

src/databricks/labs/ucx/source_code/linters/python_ast.py

@@ -192,191 +198,29 @@ def _get_attribute_value(cls, node: Attribute):
            logger.debug(f"Missing handler for {name}")
        return None

-    def infer_values(self, state: CurrentSessionState | None = None) -> Iterable[InferredValue]:


this change is a bit more difficult to review, as methods are moved, the reasoning is not clear

code is moved to work around cyclic imports

nfx · 2024-07-05T19:35:04Z

src/databricks/labs/ucx/source_code/linters/python_infer.py

@@ -0,0 +1,221 @@
+from __future__ import annotations


don't mix refactoring and new features in the same PR. it's completely unclear to me what changed in these changes and what's the new code. please revert the introduction of this module in this PR.

this was moved from python_ast to work around cyclic imports

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

…notebook-run-call # Conflicts: # src/databricks/labs/ucx/source_code/base.py # src/databricks/labs/ucx/source_code/known.json # src/databricks/labs/ucx/source_code/linters/imports.py # src/databricks/labs/ucx/source_code/linters/python_ast.py # src/databricks/labs/ucx/source_code/notebooks/sources.py # tests/unit/source_code/linters/test_python_ast.py # tests/unit/source_code/test_notebook_linter.py

…notebook-run-call # Conflicts: # src/databricks/labs/ucx/source_code/base.py # src/databricks/labs/ucx/source_code/jobs.py # src/databricks/labs/ucx/source_code/known.py # src/databricks/labs/ucx/source_code/linters/files.py # src/databricks/labs/ucx/source_code/notebooks/cells.py # src/databricks/labs/ucx/source_code/notebooks/sources.py # tests/unit/source_code/notebooks/test_sources.py # tests/unit/source_code/test_functional.py # tests/unit/source_code/test_notebook_linter.py

github-actions · 2024-07-09T10:30:55Z

✅ 160/160 passed, 1 flaky, 6 skipped, 3h39m15s total

Flaky tests:

🤪 test_experimental_permissions_migration_for_group_with_same_name (2m15.958s)

_{Running from acceptance #4548}

…notebook-run-call

ericvergnaud · 2024-07-10T11:11:19Z

@nfx is something still blocking this PR?

nfx

please verify in the real workspace

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

src/databricks/labs/ucx/source_code/notebooks/cells.py

src/databricks/labs/ucx/source_code/notebooks/sources.py

nfx · 2024-07-10T12:21:03Z

src/databricks/labs/ucx/source_code/notebooks/sources.py

+            yield from self._mutate_path_lookup(base_node)
+            return
+        if isinstance(base_node, NotebookRunCall):
+            has_unresolved, paths = base_node.get_notebook_paths(self._session_state)


are you 100% sure this behavior is necessary? dbutils.notebook.run() does not propagate variables to the parent notebook, only %run does. provide a screenshot from a real workspace environment to confirm this.

you're right, that was an incorrect assumption

nfx · 2024-07-10T12:22:29Z

tests/unit/source_code/samples/values_across_notebooks_dbutils_notebook_run.py

+# Databricks notebook source
+
+dbutils.notebook.run("./values_across_notebooks_child.py")
+spark.table(f"{a}")


are you sure this is the right logic? my hunch it does not compute on workspace

ericvergnaud · 2024-07-10T14:24:27Z

closing in favor of #2091

ericvergnaud and others added 15 commits July 2, 2024 16:15

append statements

7a3170f

reuse python linters across cells

e56dda6

resolve cyclic import

fa9fb00

fix merge issue

c8d05b0

Merge branch 'main' into infer-values-across-notebook-cells

f364b02

# Conflicts: # src/databricks/labs/ucx/source_code/linters/context.py

successfully infers values from child notebook

c3cbc7b

successfully infers values from child notebook in run cell

56e3530

formatting

d9d3172

formatting

1e5feff

PythonSequentialLinter is stateful so instantiate it when required

257fea6

Merge branch 'main' into infer-values-across-notebook-cells

dacf50a

Merge branch 'infer-values-across-notebook-cells' into infer-values-f…

65072d6

…rom-child-notebook-in-run-cell

more tests

d244a5a

peogress

1e9ed6a

infer values from child notebooks loaded via dbutils.notebook.run

d6d9032

ericvergnaud requested review from a team and HariGS-DB July 4, 2024 15:34

ericvergnaud had a problem deploying to account-admin July 4, 2024 15:34 — with GitHub Actions Error

fix crasher

d8089f0

ericvergnaud had a problem deploying to account-admin July 4, 2024 15:59 — with GitHub Actions Error

fix crasher

6c7f715

ericvergnaud had a problem deploying to account-admin July 4, 2024 16:09 — with GitHub Actions Failure

ericvergnaud mentioned this pull request Jul 5, 2024

Infer values from child notebook in magic line #2091

Merged

1 task

nfx requested changes Jul 5, 2024

View reviewed changes

nfx added the pr/do-not-merge this pull request is not ready to merge label Jul 5, 2024

ericvergnaud added 2 commits July 8, 2024 09:57

rename method

e00d9fd

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

ericvergnaud temporarily deployed to account-admin July 9, 2024 10:11 — with GitHub Actions Inactive

ericvergnaud enabled auto-merge July 9, 2024 11:14

Merge branch 'main' into infer-values-from-child-notebook-in-dbutils-…

fb358bf

…notebook-run-call

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

Merge branch 'main' into infer-values-from-child-notebook-in-dbutils-…

a6ac282

…notebook-run-call

ericvergnaud temporarily deployed to account-admin July 10, 2024 09:49 — with GitHub Actions Inactive

Merge branch 'main' into infer-values-from-child-notebook-in-dbutils-…

5a8512d

…notebook-run-call

ericvergnaud temporarily deployed to account-admin July 10, 2024 11:10 — with GitHub Actions Inactive

nfx requested review from asnare and JCZuurmond and removed request for HariGS-DB July 10, 2024 12:16

nfx removed the pr/do-not-merge this pull request is not ready to merge label Jul 10, 2024

nfx requested changes Jul 10, 2024

View reviewed changes

address comments

bea4042

ericvergnaud temporarily deployed to account-admin July 10, 2024 13:39 — with GitHub Actions Inactive

ericvergnaud closed this Jul 10, 2024

auto-merge was automatically disabled July 10, 2024 14:24
Pull request was closed

ericvergnaud deleted the infer-values-from-child-notebook-in-dbutils-notebook-run-call branch July 11, 2024 14:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Infer values from child notebook loaded via dbutils.notebook.run() call #2083

Infer values from child notebook loaded via dbutils.notebook.run() call #2083

ericvergnaud commented Jul 4, 2024 •

edited

Loading

nfx left a comment

nfx Jul 5, 2024

ericvergnaud Jul 8, 2024

nfx Jul 5, 2024

ericvergnaud Jul 8, 2024 •

edited

Loading

nfx Jul 5, 2024

ericvergnaud Jul 8, 2024

github-actions bot commented Jul 9, 2024 •

edited

Loading

ericvergnaud commented Jul 10, 2024

nfx left a comment

nfx Jul 10, 2024

ericvergnaud Jul 10, 2024

nfx Jul 10, 2024

ericvergnaud Jul 10, 2024

ericvergnaud commented Jul 10, 2024

	def mutate_path_lookup(self, path_lookup: PathLookup):
	def apply_to(self, path_lookup: PathLookup):

Infer values from child notebook loaded via dbutils.notebook.run() call #2083

Infer values from child notebook loaded via dbutils.notebook.run() call #2083

Conversation

ericvergnaud commented Jul 4, 2024 • edited Loading

Changes

Linked issues

Functionality

Tests

nfx left a 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

ericvergnaud Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 9, 2024 • edited Loading

ericvergnaud commented Jul 10, 2024

nfx left a 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

ericvergnaud commented Jul 10, 2024

ericvergnaud commented Jul 4, 2024 •

edited

Loading

ericvergnaud Jul 8, 2024 •

edited

Loading

github-actions bot commented Jul 9, 2024 •

edited

Loading