DM-44397: Run all notebooks in repo directory #347

fajpunk · 2024-05-19T20:17:25Z

NotebookRunner business now runs all notebooks in a repo by default
Except in the mobu_exclude or its descendants
Add exclude_dirs option to NotebookRunner business to list
additional directories in which notebooks will NOT be run
Phalanx mobu docs updated here.

Before this goes out, we'll need to update the tutorial notebooks and the system-test notebook repos to put all of the subdir notebooks in the mobu_exclude dir.

lsst-sqre/mobu#347 changes the Mobu notebook runner business to run all notebooks in all subdirs by default. This PR prevents any changes in behavior to existing flocks.

rra

Overall, looks good, thank you! Just a couple of notes and suggestions.

rra · 2024-05-20T16:41:02Z

src/mobu/constants.py

+NOTEBOOK_ALWAYS_EXCLUDE_DIRS = {"mobu_exclude"}
+"""NotebookRunner always excludes notebooks in these dirs."""
+


This feels a little magical and hard-coded to me. I think I would lean towards not excluding any directories by default and making people add an excluded directory if they want one. But maybe I'm missing a use case for always having a default excluded directory?

Agreed it seems magical. There are a couple of issues:

One of the things we want is for folks to be able to manage these excluded notebooks without having to make phalanx PRs. IIUC, all of the flock config is defined in phalanx, so folks would have to make phalanx PRs as they add and remove excluded dirs (or make one phalanx MR to get their own single exclude dir like in this magic default)

A magic default exclude dir gets us out of having to tightly coordinate this mobu release with notebook repo changes. With this, we can modify the notebook repos any time before the mobu release to any environment without having to coordinate adding the exclude_dirs flock option at the same time as we release to each environment.

To address this, we could have this behavior be opt-in by adding a run_in_subdirs option that defaults to False. Then we can deploy the mobu change without affecting current behavior. We could then change the default at some point in the future when all of the flocks have their excluded dirs specified appropriately.

One bigger change we could make to address both of these issues is to have this behavior specified in a config file or something in the notebook repos themselves. Then:

We could first add that config file to the repos before deploying the mobu changes and current behavior would be kept everywhere

No phalanx changes would be needed to manage these exclude dirs

I ditched the magic dir. Seems not worth the magic and confusion. I add the subdirs in the currently existing flocks in the phalanx PR.

And I think I misunderstood the way the release process will work. The mobu version and the flock config will get synced together for each environment; there won't be a state where mobu is updated but the flock config isn't.

rra · 2024-05-20T16:42:44Z

src/mobu/services/business/notebookrunner.py

+        parents = set(notebook.parents)
+        excluded_parents = parents & self._exclude_paths
+        return len(excluded_parents) != 0


Suggested change

parents = set(notebook.parents)

excluded_parents = parents & self._exclude_paths

return len(excluded_parents) != 0

return bool(set(notebook.parents) & self._exclude_paths)

rra · 2024-05-20T16:46:21Z

tests/business/notebookrunner_test.py

-                "notebook": "test-notebook.ipynb",
+                "notebook": ANY,


I think we've now lost this test, in that nothing is now testing that the notebook data is set correctly to the executing notebook. It's some duplication, but it might be a bit cleaner to add a separate test for the recursive notebook running that uses a separate directory.

rra · 2024-05-20T16:47:14Z

tests/business/notebookrunner_test.py

@@ -59,7 +59,7 @@ async def test_run(
                    "options": {
                        "spawn_settle_time": 0,
                        "execution_idle_time": 0,
-                        "max_executions": 1,
+                        "max_executions": 10,


I think this would be a stronger test if max_executions exactly matched the number of notebooks that should be run, since then you can also test for off-by-one errors (make sure the first notebook isn't run twice, for instance).

Yeah, my change makes that test weaker, but I also wanted to ensure that no notebooks from excluded dirs would be run. I changed max_execution back to the exact number of notebooks, but I also moved the "cycle done" message from the beginning of the next cycle, to the end of execute_code, and I'm asserting that's logged.

That gets us the best of both worlds, right?

rra · 2024-05-20T16:48:24Z

tests/business/notebookrunner_test.py

+    # Set up git client
+    git = Git(repo=repo_path)
+    await git.init("--initial-branch=main")
+    await git.config("user.email", "[email protected]")
+    await git.config("user.name", "Git User")
+    for path in repo_path.iterdir():
+        if not path.name.startswith("."):
+            await git.add(str(path))
+    await git.commit("-m", "Initial commit")


Since there are now a couple copies of this code, it's probably time to move it into a helper function.

rra · 2024-05-20T16:48:57Z

tests/business/notebookrunner_test.py

+                    "options": {
+                        "spawn_settle_time": 0,
+                        "execution_idle_time": 0,
+                        "max_executions": 10,


Same note here about max_executions.

rra

This looks great. Just a couple of minor nits on the change log entry.

rra · 2024-05-20T21:21:06Z

changelog.d/20240520_113001_danfuchs_notebook_directories.md

+
+### Backwards-incompatible changes
+
+- NotebookRunner business now runs all notebooks in a repo by default except in the `mobu_exclude` or its descendants


You dropped the mobu_exclude part. We've also been putting periods at the end of change log entries (sort of an arbitrary style thing).

rra · 2024-05-20T21:21:55Z

changelog.d/20240520_113001_danfuchs_notebook_directories.md

+### Other changes
+
+- Updated python dependencies


We don't note this in the change log because we do it with every release, so it just adds noise. Instead, we document at the start of CHANGELOG.md that we do this with every release.

Remove magic notebook exclude dir

fajpunk force-pushed the tickets/DM-44397/notebook-directories branch 2 times, most recently from 589e8ee to c870edb Compare May 19, 2024 20:18

Update dependencies

498015e

fajpunk force-pushed the tickets/DM-44397/notebook-directories branch 2 times, most recently from 86843a2 to 8b63d26 Compare May 20, 2024 01:08

fajpunk mentioned this pull request May 20, 2024

DM-44397: Mobu: disable running existing notebooks in subdirs lsst-sqre/phalanx#3346

Merged

fajpunk force-pushed the tickets/DM-44397/notebook-directories branch 2 times, most recently from f240142 to 047d465 Compare May 20, 2024 15:38

fajpunk requested review from rra and athornton and removed request for athornton May 20, 2024 15:45

fajpunk mentioned this pull request May 20, 2024

Prevent new mobu from running notebooks in subdirs lsst-sqre/system-test#84

Closed

rra reviewed May 20, 2024

View reviewed changes

Log notebook cycle done msg before next cycle starts

dfa2141

fajpunk force-pushed the tickets/DM-44397/notebook-directories branch from fac6558 to 8e3f112 Compare May 20, 2024 18:24

rra approved these changes May 20, 2024

View reviewed changes

Run all notebooks in all dirs, add exclude option

26a6535

Remove magic notebook exclude dir

fajpunk force-pushed the tickets/DM-44397/notebook-directories branch from f006b15 to 26a6535 Compare May 20, 2024 21:31

fajpunk merged commit dce6bc0 into main May 21, 2024
3 checks passed

fajpunk deleted the tickets/DM-44397/notebook-directories branch May 21, 2024 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-44397: Run all notebooks in repo directory #347

DM-44397: Run all notebooks in repo directory #347

fajpunk commented May 19, 2024 •

edited

Loading

rra left a comment

rra May 20, 2024

fajpunk May 20, 2024

fajpunk May 20, 2024

fajpunk May 20, 2024

rra May 20, 2024

rra May 20, 2024

rra May 20, 2024

fajpunk May 20, 2024

rra May 20, 2024

rra May 20, 2024

rra left a comment

rra May 20, 2024

rra May 20, 2024

		NOTEBOOK_ALWAYS_EXCLUDE_DIRS = {"mobu_exclude"}
		"""NotebookRunner always excludes notebooks in these dirs."""


		### Backwards-incompatible changes

		- NotebookRunner business now runs all notebooks in a repo by default except in the `mobu_exclude` or its descendants

DM-44397: Run all notebooks in repo directory #347

DM-44397: Run all notebooks in repo directory #347

Conversation

fajpunk commented May 19, 2024 • edited Loading

rra 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

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

rra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fajpunk commented May 19, 2024 •

edited

Loading