[TECH DEBT] Remove walked paths parameter from dependency graph walker (#3645)

## Changes
Remove walked paths from dependency graph walker as it should not be
used, the developer should relay on the
`MockPathLookup.successfully_resolved_paths` as it is important that the
paths are resolved not just added to the cache in the walker

Author: JCZuurmond

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

@@ -62,7 +62,7 @@ def lint(
             stdout.write(f"{located_advice}\n")
         return located_advices
 
-    def lint_path(self, path: Path, linted_paths: set[Path] | None = None) -> Iterable[LocatedAdvice]:
+    def lint_path(self, path: Path) -> Iterable[LocatedAdvice]:
         is_dir = path.is_dir()
         loader: DependencyLoader
         if is_a_notebook(path):
@@ -79,9 +79,7 @@ def lint_path(self, path: Path, linted_paths: set[Path] | None = None) -> Iterab
         problems = container.build_dependency_graph(graph)
         for problem in problems:
             yield problem.as_located_advice()
-        if linted_paths is None:
-            linted_paths = set()
-        walker = LintingWalker(graph, linted_paths, self._path_lookup, path.name, self._context_factory)
+        walker = LintingWalker(graph, self._path_lookup, self._context_factory)
         yield from walker
 
 

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

@@ -36,22 +36,38 @@
 
 
 class DependencyGraphWalker(abc.ABC, Generic[T]):
+    """Walks over the dependencies in a graph starting from the root dependencies going depth first.
 
-    def __init__(self, graph: DependencyGraph, walked_paths: set[Path], path_lookup: PathLookup):
+    Implemented as an object to iterate over, for example:
+    ``` python
+    walker = DependencyGraphWalker()
+    for processed_dependency_output in walker:
+        # Do something with output
+    ```
+    """
+
+    def __init__(self, graph: DependencyGraph, path_lookup: PathLookup):
         self._graph = graph
-        self._walked_paths = walked_paths
         self._path_lookup = path_lookup
-        self._lineage: list[Dependency] = []
+
+        self._walked_paths = set[Path]()
+        self._lineage = list[Dependency]()
 
     def __iter__(self) -> Iterator[T]:
+        """Iterate over the dependencies starting from the root."""
         for dependency in self._graph.root_dependencies:
             # the dependency is a root, so its path is the one to use
             # for computing lineage and building python global context
-            root_path = dependency.path
-            yield from self._iter_one(dependency, self._graph, root_path)
+            yield from self._iter_one(dependency, self._graph, dependency.path)
 
     def _iter_one(self, dependency: Dependency, graph: DependencyGraph, root_path: Path) -> Iterable[T]:
+        """Iterate over a single dependency going depth first."""
         if dependency.path in self._walked_paths:
+            # TODO: Decide to not skip dependencies that have been walked already.
+            # Open questions:
+            # - Should this come before or after the lineage logging?
+            # - When do we reach this? Also, it could mean that is coming from a different root,
+            #   which maybe needs to be processed.
             return
         self._lineage.append(dependency)
         self._walked_paths.add(dependency.path)
@@ -63,11 +79,13 @@ def _iter_one(self, dependency: Dependency, graph: DependencyGraph, root_path: P
         # missing graph problems have already been reported while building the graph
         if maybe_graph.graph:
             child_graph = maybe_graph.graph
+            # This makes the implementation depth first
             for child_dependency in child_graph.local_dependencies:
                 yield from self._iter_one(child_dependency, child_graph, root_path)
         self._lineage.pop()
 
     def _log_walk_one(self, dependency: Dependency) -> None:
+        """Possibly overwrite this method in a subclass for more specific logging"""
         logger.debug(f'Analyzing dependency: {dependency}')
 
     @abc.abstractmethod
@@ -76,37 +94,34 @@ def _process_dependency(
         dependency: Dependency,
         path_lookup: PathLookup,
         inherited_tree: Tree | None,
-    ) -> Iterable[T]: ...
+    ) -> Iterable[T]:
+        """Process a dependency."""
 
     @property
     def lineage(self) -> list[LineageAtom]:
+        """The lineage for getting to the dependency."""
         lists: list[list[LineageAtom]] = [dependency.lineage for dependency in self._lineage]
         return list(itertools.chain(*lists))
 
 
 class LintingWalker(DependencyGraphWalker[LocatedAdvice]):
+    """Lint the dependencies in the graph."""
 
-    def __init__(
-        self,
-        graph: DependencyGraph,
-        walked_paths: set[Path],
-        path_lookup: PathLookup,
-        key: str,
-        context_factory: Callable[[], LinterContext],
-    ):
-        super().__init__(graph, walked_paths, path_lookup)
-        self._key = key
+    def __init__(self, graph: DependencyGraph, path_lookup: PathLookup, context_factory: Callable[[], LinterContext]):
+        super().__init__(graph, path_lookup)
         self._context_factory = context_factory
 
     def _log_walk_one(self, dependency: Dependency) -> None:
-        logger.info(f'Linting {self._key} dependency: {dependency}')
+        """Log linting a dependency"""
+        logger.info(f"Linting dependency: {dependency}")
 
     def _process_dependency(
         self,
         dependency: Dependency,
         path_lookup: PathLookup,
         inherited_tree: Tree | None,
     ) -> Iterable[LocatedAdvice]:
+        """Lint the dependency and yield the located advices."""
         # FileLinter determines which file/notebook linter to use
         linter = FileLinter(dependency, path_lookup, self._context_factory(), inherited_tree)
         for advice in linter.lint():
@@ -121,12 +136,11 @@ class _CollectorWalker(DependencyGraphWalker[S], abc.ABC):
     def __init__(
         self,
         graph: DependencyGraph,
-        walked_paths: set[Path],
         path_lookup: PathLookup,
         session_state: CurrentSessionState,
         migration_index: TableMigrationIndex,
     ):
-        super().__init__(graph, walked_paths, path_lookup)
+        super().__init__(graph, path_lookup)
         self._linter_context = LinterContext(migration_index, session_state)
 
     def _process_dependency(

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

@@ -117,11 +117,10 @@ def _lint_job(self, job: jobs.Job) -> tuple[list[JobProblem], list[DirectFsAcces
         assert job.settings is not None
         assert job.settings.name is not None
         assert job.settings.tasks is not None
-        linted_paths: set[Path] = set()
         for task in job.settings.tasks:
             graph, advices, session_state = self._build_task_dependency_graph(task, job)
             if not advices:
-                advices = self._lint_task(task, graph, session_state, linted_paths)
+                advices = self._lint_task(graph, session_state)
             for advice in advices:
                 absolute_path = "UNKNOWN" if advice.has_missing_path() else advice.path.absolute().as_posix()
                 job_problem = JobProblem(
@@ -173,20 +172,8 @@ def _build_task_dependency_graph(
         located_advices = [problem.as_located_advice() for problem in problems]
         return graph, located_advices, session_state
 
-    def _lint_task(
-        self,
-        task: jobs.Task,
-        graph: DependencyGraph,
-        session_state: CurrentSessionState,
-        linted_paths: set[Path],
-    ) -> Iterable[LocatedAdvice]:
-        walker = LintingWalker(
-            graph,
-            linted_paths,
-            self._path_lookup,
-            task.task_key,
-            lambda: LinterContext(self._migration_index, session_state),
-        )
+    def _lint_task(self, graph: DependencyGraph, session_state: CurrentSessionState) -> Iterable[LocatedAdvice]:
+        walker = LintingWalker(graph, self._path_lookup, lambda: LinterContext(self._migration_index, session_state))
         yield from walker
 
     def _collect_task_dfsas(
@@ -199,7 +186,7 @@ def _collect_task_dfsas(
         # need to add lineage for job/task because walker doesn't register them
         job_id = str(job.job_id)
         job_name = job.settings.name if job.settings and job.settings.name else "<anonymous>"
-        for dfsa in DfsaCollectorWalker(graph, set(), self._path_lookup, session_state, self._migration_index):
+        for dfsa in DfsaCollectorWalker(graph, self._path_lookup, session_state, self._migration_index):
             atoms = [
                 LineageAtom(object_type="WORKFLOW", object_id=job_id, other={"name": job_name}),
                 LineageAtom(object_type="TASK", object_id=f"{job_id}/{task.task_key}"),
@@ -216,7 +203,7 @@ def _collect_task_tables(
         # need to add lineage for job/task because walker doesn't register them
         job_id = str(job.job_id)
         job_name = job.settings.name if job.settings and job.settings.name else "<anonymous>"
-        for used_table in TablesCollectorWalker(graph, set(), self._path_lookup, session_state, self._migration_index):
+        for used_table in TablesCollectorWalker(graph, self._path_lookup, session_state, self._migration_index):
             atoms = [
                 LineageAtom(object_type="WORKFLOW", object_id=job_id, other={"name": job_name}),
                 LineageAtom(object_type="TASK", object_id=f"{job_id}/{task.task_key}"),

tests/integration/source_code/solacc.py

@@ -16,6 +16,7 @@
 from databricks.labs.ucx.framework.utils import run_command
 from databricks.labs.ucx.hive_metastore.table_migration_status import TableMigrationIndex
 from databricks.labs.ucx.source_code.base import LocatedAdvice
+from databricks.labs.ucx.source_code.files import FileLoader
 from databricks.labs.ucx.source_code.linters.context import LinterContext
 from databricks.labs.ucx.source_code.path_lookup import PathLookup
 
@@ -106,7 +107,7 @@ class _SolaccContext:
     unparsed_files_path: Path | None = None
     files_to_skip: set[Path] | None = None
     total_count = 0
-    parseable_count = 0
+    unparseable_count = 0
     uninferrable_count = 0
     missing_imports: dict[str, dict[str, int]] = field(default_factory=dict)
     stats: list[_SolaccStats] = field(default_factory=list)
@@ -164,18 +165,17 @@ def clean_tmp_sys_paths(self):
 def _lint_dir(solacc: _SolaccContext, soldir: Path):
     path_lookup = _CleanablePathLookup()
     ws = WorkspaceClient(host='...', token='...')
+    files_to_skip = set(solacc.files_to_skip) if solacc.files_to_skip else set()
     ctx = LocalCheckoutContext(ws).replace(
         linter_context_factory=lambda session_state: LinterContext(TableMigrationIndex([]), session_state),
         path_lookup=path_lookup,
+        file_loader=FileLoader(exclude_paths=files_to_skip),
     )
     all_files = list(soldir.glob('**/*.py')) + list(soldir.glob('**/*.sql'))
     solacc.total_count += len(all_files)
-    # pre-populate linted_files such that files to skip are not linted
-    files_to_skip = set(solacc.files_to_skip) if solacc.files_to_skip else set()
-    linted_files = set(files_to_skip)
     # lint solution
     start_timestamp = datetime.now(timezone.utc)
-    advices = list(ctx.local_code_linter.lint_path(soldir, linted_files))
+    advices = list(ctx.local_code_linter.lint_path(soldir))
     end_timestamp = datetime.now(timezone.utc)
     # record stats
     stats = _SolaccStats(
@@ -189,7 +189,7 @@ def _lint_dir(solacc: _SolaccContext, soldir: Path):
     solacc.stats.append(stats)
     # collect unparseable files
     unparseables = _collect_unparseable(advices)
-    solacc.parseable_count += len(linted_files) - len(files_to_skip) - len(set(advice.path for advice in unparseables))
+    solacc.unparseable_count += len(files_to_skip) + len(set(advice.path for advice in unparseables))
     if solacc.unparsed_files_path:
         for unparseable in unparseables:
             logger.error(f"Error during parsing of {unparseable.path}: {unparseable.advice.message}".replace("\n", " "))
@@ -227,11 +227,11 @@ def _lint_repos(clone_urls, sol_to_lint: str | None):
             if os.getenv("CI"):
                 shutil.rmtree(sol_dir)
     all_files_len = solacc.total_count - (len(solacc.files_to_skip) if solacc.files_to_skip else 0)
-    parseable_pct = int(solacc.parseable_count / all_files_len * 100)
+    unparseable_pct = int(solacc.unparseable_count / all_files_len * 100)
     missing_imports_count = sum(sum(details.values()) for details in solacc.missing_imports.values())
     logger.info(
         f"Skipped: {len(solacc.files_to_skip or [])}, "
-        f"parseable: {parseable_pct}% ({solacc.parseable_count}/{all_files_len}), "
+        f"unparseable: {unparseable_pct}% ({solacc.unparseable_count}/{all_files_len}), "
         f"missing imports: {missing_imports_count}, "
         f"not computed: {solacc.uninferrable_count}"
     )
@@ -243,7 +243,7 @@ def _lint_repos(clone_urls, sol_to_lint: str | None):
             message = json.dumps(dataclasses.asdict(stats), default=str)
             stats_file.writelines([message])
     # fail the job if files are unparseable
-    if parseable_pct < 100:
+    if unparseable_pct > 0:
         sys.exit(1)
 
 

tests/unit/source_code/linters/test_files.py

@@ -130,19 +130,17 @@ def local_code_linter(mock_path_lookup, migration_index):
 def test_linter_walks_directory(mock_path_lookup, local_code_linter) -> None:
     mock_path_lookup.append_path(Path(_samples_path(SourceContainer)))
     path = Path(__file__).parent / "../samples" / "simulate-sys-path"
-    paths: set[Path] = set()
-    advices = list(local_code_linter.lint_path(path, paths))
-    assert len(paths) > 10
+    advices = list(local_code_linter.lint_path(path))
     assert not advices
+    assert len(mock_path_lookup.successfully_resolved_paths) > 10
 
 
 def test_linter_lints_children_in_context(mock_path_lookup, local_code_linter) -> None:
     mock_path_lookup.append_path(Path(_samples_path(SourceContainer)))
     path = Path(__file__).parent.parent / "samples" / "parent-child-context"
-    paths: set[Path] = set()
-    advices = list(local_code_linter.lint_path(path, paths))
-    assert len(paths) == 3
+    advices = list(local_code_linter.lint_path(path))
     assert not advices
+    assert mock_path_lookup.successfully_resolved_paths == {path, Path("parent.py"), Path("child.py")}
 
 
 def test_triple_dot_import() -> None: