diff --git a/docs/development/index.md b/docs/development/index.md index 487127f..c2b8464 100644 --- a/docs/development/index.md +++ b/docs/development/index.md @@ -35,9 +35,16 @@ class DependentModule(MVTModule): prerequisite_results = self.get_dependency_results(PrerequisiteModule) ``` -Selecting a single module also runs its transitive dependencies. If a dependency -is unavailable or the dependency graph contains a cycle, the command logs a -warning and does not run any modules. +Selecting a single module also runs its transitive dependencies. + +A module can only depend on modules the command it runs in also has. When a +declared dependency is not among them, the command logs a warning naming the +module and the missing dependency, skips that module and everything depending +on it, and runs the rest of the analysis. Selecting such a module with +`--module` therefore leaves nothing to run, which the warning explains. + +A cycle in the dependency graph is a programming error rather than a +configuration problem: the command logs a warning and runs no modules at all. ## Custom modules diff --git a/src/mvt/common/command.py b/src/mvt/common/command.py index 6111dc7..f2f211d 100644 --- a/src/mvt/common/command.py +++ b/src/mvt/common/command.py @@ -317,6 +317,81 @@ class Command: console.print("") console.print(panel) + def _skipped_modules( + self, + required: list[type[MVTModule]], + module_indexes: dict[type[MVTModule], int], + ) -> dict[type[MVTModule], tuple[type[MVTModule], type[MVTModule]]]: + """Return the modules to drop because a dependency is unavailable. + + A module declaring a dependency this command cannot provide is unable + to run, and so is every module depending on it. Dropping only those + keeps a single wrong declaration - in a module scoped to several + commands, for example - from silencing an entire analysis. + + The returned mapping gives, for each skipped module, the module which + is missing a dependency and the dependency it is missing. + """ + skipped: dict[type[MVTModule], tuple[type[MVTModule], type[MVTModule]]] = {} + # Skipping the one module a run was asked for leaves nothing to run, + # which the caller reports instead. + remainder = ( + "" if self.module_name else " The rest of the analysis will still run." + ) + + # Skipping one module can skip the modules depending on it, which the + # pass over the module list may already have gone past, so repeat the + # pass until nothing changes. + changed = True + while changed: + changed = False + for module in required: + if module in skipped: + continue + + for dependency in module.dependencies: + if dependency not in module_indexes: + skipped[module] = (module, dependency) + changed = True + self.log.warning( + "Module %s will be SKIPPED: it depends on module " + "%s, which is not available in this command.%s", + module.__name__, + dependency.__name__, + remainder, + ) + break + + if dependency in skipped: + root, missing = skipped[dependency] + skipped[module] = (root, missing) + changed = True + if dependency is root: + self.log.warning( + "Module %s will be SKIPPED: it depends on " + "module %s, itself skipped for depending on " + "unavailable module %s.%s", + module.__name__, + dependency.__name__, + missing.__name__, + remainder, + ) + else: + self.log.warning( + "Module %s will be SKIPPED: it depends on " + "skipped module %s, in a chain starting at " + "module %s, which depends on unavailable " + "module %s.%s", + module.__name__, + dependency.__name__, + root.__name__, + missing.__name__, + remainder, + ) + break + + return skipped + def _ordered_modules(self) -> Optional[list[type[MVTModule]]]: """Return enabled modules in stable topological order.""" modules = self._available_modules() @@ -329,30 +404,34 @@ class Command: else: selected = [module for module in modules if module.enabled] - required = set(selected) + required: set[type[MVTModule]] = set() pending = list(selected) while pending: module = pending.pop() + if module in required: + continue + required.add(module) for dependency in module.dependencies: - if dependency not in module_indexes: - self.log.warning( - "Module %s depends on unavailable module %s. " - "No modules will be run.", - module.__name__, - dependency.__name__, - ) - return None - if dependency not in required: - required.add(dependency) + # Unavailable dependencies are reported by _skipped_modules(). + if dependency in module_indexes: pending.append(dependency) + ordered_required = sorted(required, key=lambda module: module_indexes[module]) + skipped = self._skipped_modules(ordered_required, module_indexes) + runnable = [module for module in ordered_required if module not in skipped] + if skipped and not runnable: + self.log.warning( + "Every selected module was skipped for an unavailable " + "dependency. No modules will be run." + ) + dependents: dict[type[MVTModule], list[type[MVTModule]]] = { - module: [] for module in required + module: [] for module in runnable } - indegree = {module: 0 for module in required} - for module in required: + indegree = {module: 0 for module in runnable} + for module in runnable: for dependency in module.dependencies: - if dependency not in required: + if dependency not in indegree: continue dependents[dependency].append(module) indegree[module] += 1 @@ -371,7 +450,7 @@ class Command: if indegree[dependent] == 0: heappush(ready, (module_indexes[dependent], dependent)) - if len(ordered) != len(required): + if len(ordered) != len(runnable): cyclic_modules = sorted( (module.__name__ for module, count in indegree.items() if count > 0) ) diff --git a/tests/common/test_command.py b/tests/common/test_command.py index 4dbfe1a..4835fd3 100644 --- a/tests/common/test_command.py +++ b/tests/common/test_command.py @@ -157,7 +157,7 @@ class TestCommand: assert not hasattr(cmd, "initialized") assert "Circular module dependency detected" in caplog.text - def test_unavailable_dependency_warns_and_stops(self, caplog): + def test_unavailable_dependency_only_skips_the_dependent_module(self, caplog): class UnavailableModule(RecordingModule): pass @@ -165,14 +165,97 @@ class TestCommand: dependencies = (UnavailableModule,) cmd = RecordingCommand() - cmd.modules = [DependentModule] + cmd.modules = [DependentModule, IndependentModule, FirstModule] + + with caplog.at_level(logging.WARNING): + cmd.run() + + assert RecordingModule.run_order == ["IndependentModule", "FirstModule"] + assert cmd.initialized + assert "Module DependentModule will be SKIPPED" in caplog.text + assert "depends on module UnavailableModule" in caplog.text + + def test_modules_depending_on_a_skipped_module_are_skipped_too(self, caplog): + class UnavailableModule(RecordingModule): + pass + + class SkippedModule(RecordingModule): + dependencies = (UnavailableModule,) + + class DependsOnSkippedModule(RecordingModule): + dependencies = (SkippedModule,) + + class DependsOnTheChain(RecordingModule): + dependencies = (DependsOnSkippedModule,) + + cmd = RecordingCommand() + cmd.modules = [ + DependsOnTheChain, + DependsOnSkippedModule, + SkippedModule, + IndependentModule, + ] + + with caplog.at_level(logging.WARNING): + cmd.run() + + assert RecordingModule.run_order == ["IndependentModule"] + skip_warnings = [ + record.getMessage() + for record in caplog.records + if "will be SKIPPED" in record.getMessage() + ] + assert len(skip_warnings) == 3 + assert [warning.split()[1] for warning in skip_warnings] == [ + "SkippedModule", + "DependsOnSkippedModule", + "DependsOnTheChain", + ] + # Every warning names the root cause: the module missing a dependency + # and the dependency it is missing. + assert all("UnavailableModule" in warning for warning in skip_warnings) + assert all("module SkippedModule" in warning for warning in skip_warnings[1:]) + + def test_explicitly_selected_module_with_missing_dependency_runs_nothing( + self, caplog + ): + class UnavailableModule(RecordingModule): + pass + + class DependentModule(RecordingModule): + dependencies = (UnavailableModule,) + + cmd = RecordingCommand(module_name="DependentModule") + cmd.modules = [DependentModule, IndependentModule] with caplog.at_level(logging.WARNING): cmd.run() assert RecordingModule.run_order == [] - assert not hasattr(cmd, "initialized") - assert "depends on unavailable module UnavailableModule" in caplog.text + assert "Module DependentModule will be SKIPPED" in caplog.text + assert "No modules will be run" in caplog.text + # Nothing else was selected, so the warning must not promise that the + # analysis continues right before saying that it does not. + assert "The rest of the analysis will still run" not in caplog.text + + def test_unaffected_dependency_chains_keep_their_order(self, caplog): + class UnavailableModule(RecordingModule): + pass + + class SkippedModule(RecordingModule): + dependencies = (UnavailableModule,) + + cmd = RecordingCommand() + cmd.modules = [ThirdModule, SkippedModule, SecondModule, FirstModule] + + with caplog.at_level(logging.WARNING): + cmd.run() + + assert RecordingModule.run_order == [ + "FirstModule", + "SecondModule", + "ThirdModule", + ] def test_custom_modules_are_filtered_before_ordering(self): cmd = RecordingCommand()