-
Notifications
You must be signed in to change notification settings - Fork 423
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
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 |
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.
Looks like you got the initial changes made easily. Minor changes needed. We can talk about the type inferencing particulars tomorrow.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use Seq
instead of List
.
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.
Oh i missed this one… can you explain why?
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.
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): |
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.
/s/python/scala/
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.
fixed
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to update with an nbconvert >=
constraint to our reqs for the contract change?
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.
Maybe separate PR for the nbconvert fixes?
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.""" |
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.
/s/R/scala/
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.
fixed with 82c2328
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.