Add basic scala parameterisation support #140
Conversation
Codecov Report
@@ Coverage Diff @@
## master #140 +/- ##
==========================================
+ Coverage 74.01% 74.29% +0.28%
==========================================
Files 9 9
Lines 966 996 +30
==========================================
+ Hits 715 740 +25
- Misses 251 256 +5 |
papermill/execute.py
Outdated
| def _translate_type_list_scala(val): | ||
| """Translate list to scala.""" | ||
| escaped = ', '.join([_translate_type_r(v) for v in val]) | ||
| return 'List({})'.format(escaped) |
There was a problem hiding this comment.
Oh i missed this one… can you explain why?
There was a problem hiding this comment.
Note: as of 807f8e7 this is fixed too.
papermill/execute.py
Outdated
| @@ -283,6 +315,13 @@ def build_r_params(parameters): | |||
| param_content += '{} = {}\n'.format(var, _translate_type_r(val)) | |||
| return param_content | |||
|
|
|||
| @register_param_builder("scala") | |||
| def build_python_params(parameters): | |||
papermill/preprocess.py
Outdated
| @@ -3,8 +3,7 @@ | |||
| import datetime | |||
|
|
|||
| from concurrent import futures | |||
| from nbconvert.preprocessors import ExecutePreprocessor | |||
There was a problem hiding this comment.
Do we need to update with an nbconvert >= constraint to our reqs for the contract change?
There was a problem hiding this comment.
Maybe separate PR for the nbconvert fixes?
There was a problem hiding this comment.
This isn't a change in nbconvert, you could have done this at least as of nbconvert 5.3.0
There was a problem hiding this comment.
sure I'll make this a separate PR.
papermill/execute.py
Outdated
| """Writers parameter assignment code for Python kernels.""" | ||
| param_content = "// Parameters\n" | ||
| for var, val in parameters.items(): | ||
| param_content += 'val {} = {}\n'.format(var, _translate_type_scala(val)) |
There was a problem hiding this comment.
I don't think we can do much about it in the general case but I know someone's going to immediately complain that we don't have strict typing here (i.e. Map[String, Any] and Seq[Any]) and that the types can change from run to run.
We may need to consider also passing the raw JSON string in duplicated params for scala to enable easy play or circe translations.
There was a problem hiding this comment.
I don't think that we're going to be able to handle that with the current type system that papermill implements.
could you link to what you're talking about re: play or circe?
There was a problem hiding this comment.
Agreed. Let's hash out tomorrow if we want to add string version of each JSON attribute so users can choose to parse with their own parsers if they choose.
https://www.playframework.com/documentation/2.6.x/ScalaJson
and
https://circe.github.io/circe/
84eea52
to
7b3b9ea
Compare
|
@MSeal Addressed your comments except for the one that would require the much richer type system which I think is outside of the scope of this PR. |
papermill/execute.py
Outdated
| return 'Seq({})'.format(escaped) | ||
|
|
||
| def _translate_type_scala(val): | ||
| """Translate each of the standard json/yaml types to appropiate objects in R.""" |
I still need to add tests and make sure all the data types are correctly specified, butthis worked on the command line with strings.Tests have now been added & data types are tested.