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-45079: Exclude notebooks if required services are not provisioned #359

Closed
wants to merge 7 commits into from

Conversation

fajpunk
Copy link
Member

@fajpunk fajpunk commented Jul 19, 2024

This got bigger than expected, as usual. I'm glad to break it up into separate PRs for subsets of the commits.

Don't run notebooks in environments that don't have the necessary services

  • Add a list of available services to app config (to be replaced by some kind of service discovery at some point)
  • Parse a 'mobu' section out of notebook metadata
  • Don't run the notebook if it declares required services in the metadata and the services aren't available in the environment.

Putting the required services in the notebook metadata can be done in JupyterLab, though is not the nicest UX for this. We now have an in-repo config file (mobu.yaml), maybe it would be better to keep a manifest in there? I like having it closer to the notebook though. I'm open to other suggestions.

Consolidate all notebook filtering logic to the business

... instead of being split/duplicated in the business and the CI worker.

  • This is nice because it is easier to understand and less error prone
  • This stinks because a job that didn't find any notebooks to run will look just like a job that successfully ran notebooks

I'd like to mark the check run as neutral in this case, but I think it can wait until another PR.

There are a few ways to do it:

  • Inspect the log output in the SolitaryResult. The more I think about this, given this notebook runner is the only case of this third type of result from a solitary execution (aside from 'success' or 'failure'), the less opposed I am to do it...
  • Factor the filtering logic out and use it in both the CI worker and in the NotebookRunner. I don't love this, because:
    • it requires looking at the contents of all of the files in both places, which isn't horrible I guess
    • We'd have to remember to factor out any future logic that gets added to NotebookRunner outside of the filtering that causes a wacky situation like this
  • Thread a result type through the solitary execution path and have the NotebookRunner return a SolitaryNeutralResult. This is probably the right way to do it, but it would add a lot of changes to this already-big-ish PR.

Another related thing would be putting execution logs in the github check details. I haven't done this so far because I'm worried that this would make it easy to accidentally leak some kind of sensitive info.

Testing

  • Notebooks with required services metadata don't get run
  • In-repo exclude_dirs overrides flock exclude_dirs
  • CI runs where all notebooks are filtered succeed

@fajpunk fajpunk force-pushed the tickets/DM-45079/service-specific-checks branch from d18aa94 to ed6ec1f Compare July 19, 2024 18:00
…vices

* Parse a 'mobu' section out of notebook metadata
* Add a list of available services to app config (to be replaced by some
kind of service discovery at some point
* Don't run the notebook if it declares required services in the
metadata and the services aren't available in the environment.
@fajpunk fajpunk force-pushed the tickets/DM-45079/service-specific-checks branch 2 times, most recently from 07a475b to a49e8f6 Compare July 19, 2024 18:56
@fajpunk fajpunk requested a review from rra July 19, 2024 18:56
@fajpunk fajpunk force-pushed the tickets/DM-45079/service-specific-checks branch from a49e8f6 to a3c2fd9 Compare July 19, 2024 19:01
@fajpunk fajpunk removed the request for review from rra July 19, 2024 19:04
@fajpunk
Copy link
Member Author

fajpunk commented Jul 19, 2024

Gonna split this up into smaller PRs.

@fajpunk fajpunk closed this Jul 19, 2024
@fajpunk fajpunk mentioned this pull request Jul 19, 2024
@fajpunk fajpunk deleted the tickets/DM-45079/service-specific-checks branch September 4, 2024 14:44
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.

1 participant