-
Notifications
You must be signed in to change notification settings - Fork 429
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
Guess params by introspecting the _parameters_ cell #531
Conversation
Codecov Report
@@ Coverage Diff @@
## main #531 +/- ##
==========================================
+ Coverage 91.77% 92.30% +0.52%
==========================================
Files 14 16 +2
Lines 1289 1403 +114
==========================================
+ Hits 1183 1295 +112
- Misses 106 108 +2 |
Thanks for posting the PR -- I just got back and will get a chance to go through it in the next day or two |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for building this out. There's some comments to address and probably a little bit of rearrangement needed to avoid code paths when executing but I think the core implementation idea is solid in approach.
|
||
class PythonTranslator(Translator): | ||
# Pattern to capture parameters within cell input | ||
PARAMETER_PATTERN = re.compile( | ||
r"^(?P<target>\w[\w_]*)\s*(:\s*[\"']?(?P<annotation>\w[\w_\[\],\s]*)[\"']?\s*)?=\s*(?P<value>.*?)(\s*#\s*(type:\s*(?P<type_comment>[^\s]*)\s*)?(?P<help>.*))?$" # noqa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice actually -- it even extracts the comment on the end of foo = bar # a comment
!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other way to do this for Python is to use the ast
module (e.g. https://www.mattlayman.com/blog/2018/decipher-python-ast/) and extracting the assignments programmatically. However this won't work cross-language and could even hiccup with ipython specific capabilities with python kernels (e.g. !run-something
). So I think regex is the KISS way to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is exactly my note in the PR description 😉
Note: I use a regex instead of ast for Python for two reasons: ast removes all comments but I use them to get a help comment. ast parsing for non-python language won't be available. So it seems better to use regex for all languages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep just re-enforcing the idea
return mock | ||
|
||
|
||
@pytest.mark.parametrize( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably want more examples (maybe by supplying the parameter cell directly to the inspect call in a different test parameterization) with a variety of code arrangements to prove your comment compressing code all works as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually done in papermill/tests/test_translators.py:test_inspect_python (at line 112)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the improvement and the quick turn around on the review!
This is a reboot of #158
It adds a new option
--help-notebook
to create a new cli usage:Due to that I had to remove the
required
statement on OUTPUT_PATH. But the test is still done within thepapermill
function to keep the behavior if--help-notebook
is not set.Fixes #225 => in particular the default parameters resulting of the code introspection are added as a dictionary in metadata
nb.metadata['papermill']['default_parameters']
.For now this add the ability for Python. I'm not familiar to the other languages to build the proper regex.
A follow up could be to address #55, but there is a important issue on how to fairly evaluate the variable type for non-python language.