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-44461: Refresh notebooks #349

Merged
merged 6 commits into from
May 30, 2024
Merged

Conversation

fajpunk
Copy link
Member

@fajpunk fajpunk commented May 22, 2024

  • Add a refreshing attribute to Businesses, with an endpoint and a MonkeyFlocker command to set it. Businesses can handle it however they want.
  • NotebookRunner businesses handle this by recloning their notebook repo and forcing a new session.
  • Add a GitHub webhook handler to set the refreshing attribute on NotebookRunner businesses for push events from matching repos.
  • Update dependencies

I originally wanted to handle this by stopping and starting the flock via the flock manager, but I kept running into conflicts between multiple restart requests, and conflicts between restart requests and the business-as-usual starting and stopping of Jupyter labs and sessions during normal NotebookRunner flock executions. I couldn't work through all of these in the time that I gave myself, so I ditched that plan in favor of what you see here.

@fajpunk fajpunk marked this pull request as ready for review May 22, 2024 17:27
@fajpunk fajpunk force-pushed the tickets/DM-44145/notebook-refresh branch 6 times, most recently from 1264a90 to 92e6607 Compare May 23, 2024 17:40
@fajpunk fajpunk changed the title DM-44145: Refresh notebooks DM-44461: Refresh notebooks May 23, 2024
@fajpunk fajpunk force-pushed the tickets/DM-44145/notebook-refresh branch 11 times, most recently from 0830cfa to cd47fee Compare May 29, 2024 20:15
@athornton
Copy link
Member

Typing error:

src/mobu/handlers/external.py:264: error: Missing return statement [return]

Usually before pushing a commit I like to do tox -e typing, tox -e lint, and tox -e py in that order (because that tends to be least-to-most-time-consuming-to-fix, in order).

@fajpunk fajpunk force-pushed the tickets/DM-44145/notebook-refresh branch from cd47fee to d56f90c Compare May 29, 2024 20:27
tests/handlers/github_test.py Outdated Show resolved Hide resolved
src/mobu/handlers/external.py Outdated Show resolved Hide resolved
@fajpunk fajpunk requested a review from rra May 29, 2024 20:33
@fajpunk fajpunk force-pushed the tickets/DM-44145/notebook-refresh branch 2 times, most recently from 94ad359 to 217ff73 Compare May 29, 2024 20:50
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.

Overall, this looks great! I like how the refreshing works.

requirements/dev.in Outdated Show resolved Hide resolved
requirements/main.in Outdated Show resolved Hide resolved
src/mobu/handlers/external.py Outdated Show resolved Hide resolved
src/mobu/handlers/external.py Outdated Show resolved Hide resolved
src/mobu/handlers/external.py Outdated Show resolved Hide resolved
src/mobu/services/business/notebookrunner.py Outdated Show resolved Hide resolved
src/mobu/services/business/notebookrunner.py Outdated Show resolved Hide resolved
src/mobu/services/flock.py Show resolved Hide resolved
tests/handlers/github_test.py Outdated Show resolved Hide resolved
tests/support/data/github_webhook.tmpl.json Outdated Show resolved Hide resolved
@fajpunk fajpunk force-pushed the tickets/DM-44145/notebook-refresh branch 3 times, most recently from f055dbc to 073e96b Compare May 30, 2024 01:27
@fajpunk fajpunk force-pushed the tickets/DM-44145/notebook-refresh branch from 073e96b to fc010b6 Compare May 30, 2024 02:02
@fajpunk fajpunk requested a review from rra May 30, 2024 17:31
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.

Needs a scriv file for the change log and I noticed a couple of very minor nits, but otherwise looks great!

tests/handlers/github_test.py Outdated Show resolved Hide resolved
tests/handlers/github_test.py Outdated Show resolved Hide resolved
src/mobu/constants.py Outdated Show resolved Hide resolved
@fajpunk fajpunk force-pushed the tickets/DM-44145/notebook-refresh branch from fc010b6 to 3f3a206 Compare May 30, 2024 20:30
@fajpunk fajpunk merged commit 188df6a into main May 30, 2024
3 checks passed
@fajpunk fajpunk deleted the tickets/DM-44145/notebook-refresh branch May 30, 2024 20:33
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.

3 participants