Fix to make IPerl notebooks running in notebook-http mode #271

gingerhot · 2017-12-23T04:41:25Z

When I try the notebook-http mode with the IPerl kernel and run into a constant variable assignment problem.

In Perl when we use a all-uppercase name like REQUEST, it means the variable is a constant. An assignment like REQUEST = ... is illegal, i.e. we can't use a "=" to assign a value to a constant variable.

And in fact in Ruby the uppercase name is treated as a constant, too. But in Ruby the syntax REQUEST = ... is a valid assignment, it seems there's no problem with Ruby. And nor do Python.

So here I add a kernel dependent change to the request_format function. When the kernel is iperl, it takes $REQUEST as a variable that could be assigned a value with =. And the way may be useful for some other kernels in such a situation.

And I test it with a IPerl notebook and it works.

kevin-bates · 2017-12-23T19:41:44Z

I'm not familiar with NotebookAPIHandler, but I don't think we want to use kernel_name as an indicator of language. In this context, kernel_name is essentially the name of the directory in which the kernel.json file resides. Many installations use directory names to reflect other aspects of the kernel's environment, so it's not a reliable indicator of language, which is really what you're after here. I think you really want the language field of the seed_kernelspec (which is the value of kernel_name in the handler).

One approach would be to add a sibling property on SeedingMappingKernelManager named something like seed_kernel_language that uses the seed_kernelspec property value and self.kernel_spec_manager.get_kernel_spec() to get the actual kernel spec and lookup the language (you'll want to add exception handling since issues here shouldn't fail the request). This would then get added to the handler_args dict in create_request_handlers, NotebookAPIHandler's initialization method and on to the format_request() method.

Since it doesn't appear kernel_name is used within the current NotebookAPIHandler, I would recommend just replacing it with kernel_language.

Please make the additional parameter optional since I don't think language is required in kernel.json.

I hope that helps.

gingerhot · 2017-12-24T10:01:09Z

@kevin-bates Thanks for your reply in detail and it's a great help to me. 👍

I agree that language specific is more reasonable than kernel_name dependent.

My current change is like an instant hack, I'll make a refactoring of it.

… kernel_name

kevin-bates · 2017-12-24T17:52:27Z

Thanks for the updates @gingerhot. These changes look fine to me. kernel_name could be removed since its not used, but I could see it useful for debug.

The py 2.7 failure appears to be related to a timeout and not the changes - as best as I can tell.

gingerhot · 2017-12-24T18:28:14Z

@kevin-bates Thanks for your reply. And yes, I search the codes for kernel_name and not find its use, but I'm not sure there's other purpose in design. So keep it currently?

I checked py 2.7 failure and also think maybe not a problem of my commit as local make test all passed. Hope someone in admin can help to check it.

rolweber · 2017-12-27T07:04:24Z

kernel_gateway/notebook_http/request_utils.py

    """Creates an assignment statement of bundle JSON-encoded to a variable
-    named `REQUEST`.
+    named `REQUEST` by default or kernel_language specific.

    Returns
    -------
    str
        `REQUEST = "<json-encoded expression>"`


This line of the doc string is no longer accurate after the change, right?

I think it also accurate, too. It'll keep the variable as REQUEST for those languages works well and a similar variable name for those kernel_language specific ones. I think the description is consistent to following codes logically.

Line 22 says that REQUEST = is returned, unconditionally.
Line 17 says that the return value can be different, depending on the kernel language.
That's a contradiction.

Yes, Line 22 should be updated.

rolweber · 2017-12-27T07:09:00Z

kernel_gateway/notebook_http/__init__.py

@@ -122,7 +122,8 @@ def create_request_handlers(self):
            handler_args = { 'sources' : verb_source_map,
                'response_sources' : response_source_map,
                'kernel_pool' : self.kernel_pool,
-                'kernel_name' : self.parent.kernel_manager.seed_kernelspec
+                'kernel_name' : self.parent.kernel_manager.seed_kernelspec,
+                'kernel_language' : getattr(self.parent.kernel_spec_manager.get_kernel_spec(self.parent.kernel_manager.seed_kernelspec), 'language', 'no_spec')


I'm not sure that this is a good place for the lookup. Can't the handler do the lookup itself?
If there is a need to set kernel_language from outside in some cases, then give it a default of null or the empty string, and let the handler deal with that. If there is no need to set kernel_language from outside, then don't add it to the handler_args at all.

I think it as same as other args in handler_args, that's why we construct a handler_args here, we just pass the args what we need instead of a some object and then get just a needed attribute from it.

And I read through the code once more and find that we use the lang extracted from notebook's metadata to determine the comment sign:
https://github.com/jupyter/kernel_gateway/blob/05e714805e5e74e1b4dbcbe463e4ab8ac295c490/kernel_gateway/notebook_http/__init__.py#L65-L69

So I think maybe we should also use this same lang to keep them consistent. And here we can make lang as an instance variable.

The difference to the other args is that kernel_language is dependent on kernel_name.

I ever thought that kernel_language is dependent on kernel_name, but now I don't think so. Since I found that we can get the language directly like the above mentioned:

https://github.com/jupyter/kernel_gateway/blob/05e714805e5e74e1b4dbcbe463e4ab8ac295c490/kernel_gateway/notebook_http/__init__.py#L65-L69

And I think we should keep it consistent, i.e. get the language from the same source, the notebook's metadata.

Oh, I see. That's one of the cases where I was missing the big picture :-)
Yes, the common source of information is the notebook, not the kernel_name.
Then it makes sense to pass both values as separate arguments.

rolweber · 2017-12-27T07:10:18Z

kernel_gateway/notebook_http/handlers.py

@@ -35,6 +35,8 @@ class NotebookAPIHandler(TokenAuthorizationMixin,
    kernel_name
        Kernel spec name used to launch the kernel pool. Identifies the
        language of the source code cells.


If kernel_language is added, the second sentence in the description of kernel_name is no longer accurate.

Yes, if kernel_language is decided to be added, then this description should be updated.

rolweber · 2017-12-27T07:14:03Z

kernel_gateway/notebook_http/handlers.py

@@ -45,11 +47,12 @@ class NotebookAPIHandler(TokenAuthorizationMixin,
    services.cell.parser.APICellParser for detail about how the source cells
    are identified, parsed, and associated with HTTP verbs and paths.
    """
-    def initialize(self, sources, response_sources, kernel_pool, kernel_name):
+    def initialize(self, sources, response_sources, kernel_pool, kernel_name, kernel_language='no_spec'):


Why are you using a magic value 'no_spec' as the default, instead of null or an empty string? I don't see a check for this magic value in your changes. If this "language" has now a special meaning, it would have to be documented somewhere.

I intended to add it as a self-explanatory value, or maybe there's a need of checking on it in further development. I think an empty string is ok, too.

rolweber · 2017-12-27T07:16:15Z

kernel_gateway/notebook_http/request_utils.py

@@ -12,17 +12,20 @@
 APPLICATION_JSON = 'application/json'
 TEXT_PLAIN = 'text/plain'

-def format_request(bundle):
+def format_request(bundle, kernel_language='no_spec'):


Another instance of the magic value 'no_spec'. I've expressed my concerns about this in one of the other review comments.

same as above.

rolweber · 2017-12-27T07:24:33Z

Hello @gingerhot, thanks for your contribution! I've reviewed the code changes by just looking at the diff. If some of my comments are invalid in the greater context, feel free to argue against :-)

Could you please add a little unit test for the new behavior you've introduced? That would be great.

Regarding kernel_name vs. kernel_language, I think it's good to keep the kernel_name. One of my suggested changes would make it relevant anyway. But even without that, the kernel spec (as identified by the name) is the common anchor for kernel-specific information and behavior. I'd rather have it available directly, instead of just some selected information looked up from there.

gingerhot · 2017-12-27T16:34:42Z

@rolweber thanks for your review and reply, and I see and I will :-)

And I'll add some unit tests in further commits.

Do you mean keep the kernel_name as before and use it to get the language in the handler as you said in above review comment?

rolweber · 2017-12-28T06:53:19Z

@gingerhot: Great! Yes, I suggest to keep the kernel_name as an argument and do the language lookup in the handler.

gingerhot · 2017-12-28T07:09:46Z

@rolweber As I replied above that I think the kernel_language may not be dependent on the kernel_name indeed. We just can extract it as that:

https://github.com/jupyter/kernel_gateway/blob/05e714805e5e74e1b4dbcbe463e4ab8ac295c490/kernel_gateway/notebook_http/__init__.py#L65-L69

…d argument of format_request

gingerhot · 2017-12-28T08:41:24Z

@rolweber I make a refactoring based on our discussions and add two unit tests.

rolweber · 2017-12-28T08:55:37Z

kernel_gateway/tests/notebook_http/test_request_utils.py

+    def test_format_request_with_a_kernel_language_arg(self):
+        test_request = ('''{"body": "", "headers": {}, "args": {}, "path": {}}''')
+        request_code = format_request(test_request, 'perl')
+        self.assertTrue(request_code.startswith("my $REQUEST"), "Call format_request with a kernel language argument was not formatted correctly")


Thanks. Would you mind adding two more of these tests, with languages 'python' and 'scala'? Python is kind of a default language, and I've seen special treatment of Scala for the comment_prefix.

These unit tests cover the changes in format_request. I would feel safer if we also had unit tests that cover the creation of the handler, where you added the new argument.

I'll add the more languages unit tests to format_request.

But for the creation of the handler I didn't find any existed tests for it ... maybe we could add them later?

rolweber

I'm still not happy about the default 'no_spec' being used in several places, but that might just be a personal preference.
Nevertheless, I think this change is mature enough to be merged.

rolweber · 2017-12-28T09:03:30Z

@gingerhot: I have approved this PR as it is, but I'd rather wait for @parente or some other committer to have a final look. As you can see from my comments, I still have small concerns and would like someone else to decide whether these are relevant or not. I'm not firm enough in Python and this codebase to impose my gut feeling on your contribution :-)

gingerhot · 2017-12-28T09:26:40Z

@rolweber About 'no_spec', you may give your idea and I'm not insisting on it indeed. And the truth is I can't decide what is the better way to do it.

rolweber · 2017-12-28T10:33:28Z

My idea is to use either None (corresponds to null in other languages) or the empty string '' as the default.

gingerhot · 2017-12-28T10:46:21Z

I think empty string '' is better here since I needn't give a special treatment before I call the lower() in the format_request function. And I'll update it later.

rolweber · 2017-12-28T11:29:50Z

@gingerhot: Thanks for putting so much effort into this!

gingerhot · 2017-12-28T12:12:06Z

@rolweber My pleasure~ and thank you for your patience.

fix a bug for running IPerl in notebook-http mode

1d4873f

gingerhot changed the title ~~running IPerl kernel in notebook-http mode~~ Fix to make IPerl notebooks running in notebook-http mode Dec 23, 2017

fix the tests related to format_request function

86577e0

change the method format_request kernel_language dependent instead of…

4de6eff

… kernel_name

rolweber reviewed Dec 27, 2017

View reviewed changes

refactoring kernel_language variable; add some unit tests to the adde…

aeef22e

…d argument of format_request

rolweber reviewed Dec 28, 2017

View reviewed changes

rolweber approved these changes Dec 28, 2017

View reviewed changes

add more unit tests for format_request function

2bef5f8

fix some typos

a647591

update default value of kernel_language

b5a32cf

rolweber merged commit dc23390 into jupyter-server:master Dec 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to make IPerl notebooks running in notebook-http mode #271

Fix to make IPerl notebooks running in notebook-http mode #271

gingerhot commented Dec 23, 2017 •

edited

Loading

kevin-bates commented Dec 23, 2017 •

edited

Loading

gingerhot commented Dec 24, 2017 •

edited

Loading

kevin-bates commented Dec 24, 2017

gingerhot commented Dec 24, 2017 •

edited

Loading

rolweber Dec 27, 2017

gingerhot Dec 27, 2017

rolweber Dec 28, 2017

gingerhot Dec 28, 2017

rolweber Dec 27, 2017

gingerhot Dec 27, 2017 •

edited

Loading

rolweber Dec 28, 2017

gingerhot Dec 28, 2017 •

edited

Loading

rolweber Dec 28, 2017

rolweber Dec 27, 2017

gingerhot Dec 27, 2017

rolweber Dec 27, 2017

gingerhot Dec 27, 2017

rolweber Dec 27, 2017

gingerhot Dec 27, 2017

rolweber commented Dec 27, 2017

gingerhot commented Dec 27, 2017 •

edited

Loading

rolweber commented Dec 28, 2017

gingerhot commented Dec 28, 2017

gingerhot commented Dec 28, 2017 •

edited

Loading

rolweber Dec 28, 2017

gingerhot Dec 28, 2017

rolweber left a comment

rolweber commented Dec 28, 2017

gingerhot commented Dec 28, 2017 •

edited

Loading

rolweber commented Dec 28, 2017

gingerhot commented Dec 28, 2017

rolweber commented Dec 28, 2017

gingerhot commented Dec 28, 2017

Fix to make IPerl notebooks running in notebook-http mode #271

Fix to make IPerl notebooks running in notebook-http mode #271

Conversation

gingerhot commented Dec 23, 2017 • edited Loading

kevin-bates commented Dec 23, 2017 • edited Loading

gingerhot commented Dec 24, 2017 • edited Loading

kevin-bates commented Dec 24, 2017

gingerhot commented Dec 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gingerhot Dec 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gingerhot Dec 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rolweber commented Dec 27, 2017

gingerhot commented Dec 27, 2017 • edited Loading

rolweber commented Dec 28, 2017

gingerhot commented Dec 28, 2017

gingerhot commented Dec 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rolweber left a comment

Choose a reason for hiding this comment

rolweber commented Dec 28, 2017

gingerhot commented Dec 28, 2017 • edited Loading

rolweber commented Dec 28, 2017

gingerhot commented Dec 28, 2017

rolweber commented Dec 28, 2017

gingerhot commented Dec 28, 2017

gingerhot commented Dec 23, 2017 •

edited

Loading

kevin-bates commented Dec 23, 2017 •

edited

Loading

gingerhot commented Dec 24, 2017 •

edited

Loading

gingerhot commented Dec 24, 2017 •

edited

Loading

gingerhot Dec 27, 2017 •

edited

Loading

gingerhot Dec 28, 2017 •

edited

Loading

gingerhot commented Dec 27, 2017 •

edited

Loading

gingerhot commented Dec 28, 2017 •

edited

Loading

gingerhot commented Dec 28, 2017 •

edited

Loading