Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-44397: Mobu: disable running existing notebooks in subdirs #3346

Merged
merged 3 commits into from
May 21, 2024

Conversation

fajpunk
Copy link
Member

@fajpunk fajpunk commented May 20, 2024

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.

@fajpunk fajpunk marked this pull request as draft May 20, 2024 02:26
@fajpunk fajpunk force-pushed the tickets/DM-44397/mobu-notebook-directories branch from f86c7f0 to ef84e25 Compare May 20, 2024 15:26
@fajpunk fajpunk requested review from rra and athornton May 20, 2024 15:45
Copy link
Member

@rra rra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the commit to the configuration under applications/mobu to exclude directories in the existing flocks didn't make it into the PR?

This uses the business ``NotebookRunner`` instead, which checks out a Git repository and runs all notebooks at the top level of that repository.
This uses the business ``NotebookRunner`` instead, which checks out a Git repository and runs all notebooks in that repository, at the top level and any directories and subdirectories, except:

* Notebooks in the ``mobu_exclude`` directory will never be executed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you agree with my comment on the mobu diff, also remove this sentence.

This uses the business ``NotebookRunner`` instead, which checks out a Git repository and runs all notebooks in that repository, at the top level and any directories and subdirectories, except:

* Notebooks in the ``mobu_exclude`` directory will never be executed.
* ``options.exclude_dirs`` tells mobu to not excecute any notebooks in those directories or any descendant directories.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have this line twice, here and below. I think delete this one and keep the one below?

Copy link
Member

@rra rra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one remaining nit.

The repository URL and branch are configured in ``options``.
``options.max_executions: 1`` tells mobu to shut down and respawn the pod after each notebook.
This exercises pod spawning more frequently, but does not test the lab's ability to run a long series of notebooks.
Those directories are relative to the repo root.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence seems to be in the wrong spot. I think it was supposed to be after exclude_dirs below.

@fajpunk fajpunk force-pushed the tickets/DM-44397/mobu-notebook-directories branch 2 times, most recently from e6947ee to cd4d238 Compare May 20, 2024 21:09
@fajpunk fajpunk force-pushed the tickets/DM-44397/mobu-notebook-directories branch from cd4d238 to 0f9384e Compare May 21, 2024 17:17
@fajpunk fajpunk marked this pull request as ready for review May 21, 2024 17:17
@fajpunk fajpunk enabled auto-merge May 21, 2024 17:23
@fajpunk fajpunk added this pull request to the merge queue May 21, 2024
Merged via the queue into main with commit 5822778 May 21, 2024
6 checks passed
@fajpunk fajpunk deleted the tickets/DM-44397/mobu-notebook-directories branch May 21, 2024 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants