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

Implement bash translator #674

Merged
merged 2 commits into from
Aug 14, 2022
Merged

Implement bash translator #674

merged 2 commits into from
Aug 14, 2022

Conversation

kdm9
Copy link
Contributor

@kdm9 kdm9 commented Jul 3, 2022

Support translation of parameters to bash/*NIX shell

Thanks for papermill! I have used it for python notebooks, but a lot of my work is best done in shell, for which I use jupyter notebooks with the bash_kernel (via *.sh scripts and jupytext).

Alas papermill doesn't seem to support bash/shell script, due to a hitherto unimplemented bash translator. This PR implements a BashTranslator, and therefore enables bash kernel notebooks to be parametrised. This code supports str, list, bool1, None, int, and float parameter values. Dict could in theory be supported via associative arrays but for now they are not implemented2.

For a functioning example, please see the attached notebook.

Best,
K

Footnotes

  1. Bash doesn't really have a bool type, but true and false are at least common and sensible string constants for truthy-ness and falsy-ness. See for example this SO answer.

  2. Associative arrays require quite recent bash versions, and I've never seen them used outside tutorials. Supporting them would also likely require a more involved code path as one must pre-declare an associative array in bash, and then declare one entry per line (see link above).

@kdm9
Copy link
Contributor Author

kdm9 commented Jul 3, 2022

Alas GH doesn't support attaching *.ipynb, so I have had to rename it *.txt, but here's the test notebook.

test.ipynb.txt

The following params.yml can be used with this:

say: "Write a book"
people:
  - Alice
  - Bob
  - Robert'); DROP TABLE students;-- # XKCD's favourite son

@kdm9
Copy link
Contributor Author

kdm9 commented Aug 3, 2022

A friendly ping to the nteract team about this PR. All tests pass locally, and this is currently blocking us from using papermill in our analyses.

Copy link
Member

@rohitsanj rohitsanj left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @kdm9! Can you please add some tests to test_translators.py.

@kdm9
Copy link
Contributor Author

kdm9 commented Aug 13, 2022

@rohitsanj tests implemented and passing. There are some unrelated failures on my machine, all to do with the google cloud interaction, which I assume is because I don't have a GCS environment correctly configured. Any chance you can allow the CI to run on this PR, so we can see if all is green in a well-configured environment?

@codecov
Copy link

codecov bot commented Aug 13, 2022

Codecov Report

Merging #674 (000a80a) into main (98013f0) will increase coverage by 0.13%.
The diff coverage is 90.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #674      +/-   ##
==========================================
+ Coverage   91.72%   91.86%   +0.13%     
==========================================
  Files          17       17              
  Lines        1583     1622      +39     
==========================================
+ Hits         1452     1490      +38     
- Misses        131      132       +1     
Impacted Files Coverage Δ
papermill/translators.py 98.38% <90.90%> (-0.48%) ⬇️
papermill/parameterize.py 97.91% <0.00%> (-0.05%) ⬇️
papermill/execute.py 100.00% <0.00%> (ø)
papermill/engines.py 98.37% <0.00%> (+0.09%) ⬆️
papermill/iorw.py 81.15% <0.00%> (+0.91%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98013f0...000a80a. Read the comment docs.

Copy link
Member

@MSeal MSeal left a comment

Choose a reason for hiding this comment

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

Thanks for adding tests! They passed on the CI chain so no worries about local dev issues there

@MSeal MSeal dismissed rohitsanj’s stale review August 14, 2022 22:07

Review request satisfied

@MSeal MSeal merged commit 1323836 into nteract:main Aug 14, 2022
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.

None yet

3 participants