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

Support notebooks in YAML generator (fixes #61) #62

Merged
merged 3 commits into from
Jun 30, 2021
Merged

Conversation

JuhaKiili
Copy link
Contributor

@JuhaKiili JuhaKiili commented Jun 29, 2021

This PR adds notebook source file support for the YAML generator that the Valohai CLI is calling.

You can create YAML step from Python file today:

vh yaml step hello.py

Now with valohai-utils support for notebooks, you can do the same with notebooks:

vh yaml step hello.ipynb

This will generate the YAML step that runs a notebook using Papermill. The notebook is assumed to use valohai-utils to define parameters and inputs.

Note: This PR has some copy-pasta code from Jupyhai, related to parsing source code from notebooks. We don't want valohai-utils to be dependent on Jupyhai, but Jupyhai is already dependent on valohai-utils. Once this PR is merged in, we should DRY at the Jupyhai side if possible.

@JuhaKiili JuhaKiili requested review from akx and ruksi and removed request for akx June 29, 2021 12:58
Copy link
Member

@ruksi ruksi left a comment

Choose a reason for hiding this comment

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

lgtm, some comments but nothing blocking the merge

valohai/internals/notebooks.py Show resolved Hide resolved
valohai/internals/yaml.py Show resolved Hide resolved
tests/test_parsing.py Show resolved Hide resolved
tests/test_yaml.py Show resolved Hide resolved
@ruksi ruksi merged commit a19c73f into master Jun 30, 2021
@ruksi ruksi deleted the notebook-step branch June 30, 2021 08:59
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