General cleanup take #2 by parente · Pull Request #30 · adtech-labs/spylon-kernel

parente · 2017-05-10T21:14:40Z

Comments and code cleanup in the other two modules not hit in #29

* Document scala_kernel * Rename SparkInterpreter to ScalaInterpreter for consistency with the factory function and module name

codecov · 2017-05-10T21:18:53Z

Codecov Report

Merging #30 into master will increase coverage by 0.05%.
The diff coverage is 78.37%.

@@            Coverage Diff             @@
##           master      #30      +/-   ##
==========================================
+ Coverage   84.31%   84.36%   +0.05%     
==========================================
  Files           6        6              
  Lines         408      403       -5     
==========================================
- Hits          344      340       -4     
+ Misses         64       63       -1

All or nothing. Nothing for now.

ericdill

Gods work, my friend. Gods work.

ericdill · 2017-05-11T13:13:34Z

spylon_kernel/scala_interpreter.py

+
+        Returns
+        -------
+        scala.tools.nsc.interpreter.PresentationCompilerCompleter


this looks almost fully qualified. Should this be: py4j.java_gateway.JVMView. scala.tools.nsc.interpreter.PresentationCompilerCompleter ?

scala.tools.nsc.interpreter.PresentationCompilerCompleter is the Scala type. py4j.java_gateway.JVMView is a made up namespace which allows Python code to reference them I believe. There is no real Python module+type py4j.java_gateway.JVMView. scala.tools.nsc.interpreter.PresentationCompilerCompleter.

What to do, what to do ...

Ah alright well fair enough then. Made up object paths, yey

ericdill · 2017-05-11T13:23:12Z

spylon_kernel/scala_kernel.py

-        # flag as None initially. Furthermore the metakernel will attempt to
-        # set things like _i1, _i, _ii etc.  These we dont want in the kernel
-        # for now.
        if self._scalamagic and (not name.startswith("_i")):


What are your feelings on logging attempts to set _i... variables? I'm generally 👎 on silent behavior changes but also don't have a clue how metakernel stuff works, so 🤷‍♂️

This will happen after almost every cell execution as MetaKernel will update the FIFO queue of commands and outputs, so the logs will get pretty chatty. We could log at the debug level.

ericdill · 2017-05-11T13:24:09Z

spylon_kernel/scala_kernel.py

            intp.interpret(name)
            return intp.last_result()

    def do_execute_direct(self, code, silent=False):


what is this extra kwarg?

ericdill · 2017-05-11T13:24:50Z

spylon_kernel/scala_kernel.py

+        """
        return self._scalamagic.get_completions(info)

    def get_kernel_help_on(self, info, level=0, none_on_fail=False):


another spurious kwarg?

As discussed in person, will doc how these are all implementing the expected MetaKernel interface.

ericdill · 2017-05-11T13:25:20Z

spylon_kernel/scala_kernel.py

+        level : int
+            Level of help to request, 0 for basic, 1 for more, etc.
+        none_on_fail : bool
+            Ignored


doesn't look ignored. it's passed to self._scalamagic.get_help_on()

Ignored down in _scalamagic.get_help_on. I'll try to figure out from the metakernel source exactly how it's used.

ericdill · 2017-05-11T13:39:23Z

spylon_kernel/scala_magic.py

+            We need to trim out the common pieces. Try longest prefix first, etc.
            """
            potential_prefix = os.path.commonprefix(completions)
            for i in reversed(range(len(potential_prefix)+1)):


any chance for adding some in-line comments on this code block? My brain is getting angry with me when I try and parse this

Passing on this one for now. I'll need to create some examples to really document this well.

ericdill · 2017-05-11T13:40:20Z

spylon_kernel/scala_magic.py

+        final_completions = [prefix + h[offset:] for h in completions]
+        self.kernel.log.debug('''info %s
+            completions %s
+            final %s''', info, completions, final_completions)


Do you need to dedent this? I think this will render as:

info <info> completions <completions> final <final>

but not 100% certain

The original log message had indents as well.

ericdill · 2017-05-11T13:40:47Z

spylon_kernel/scala_magic.py

+            Information returned by `metakernel.parser.Parser.parse_code`
+            including `help_obj`, etc.
+        level : int
+            Level of help to request, 0 for basic, 1 for more, etc.


how high can this number go?

Infinity. It's by convention. I'll make note of that.

ericdill · 2017-05-11T13:40:56Z

spylon_kernel/scala_magic.py

+        info : dict
+            Information returned by `metakernel.parser.Parser.parse_code`
+            including `help_obj`, etc.
+        level : int


int, optional

ericdill · 2017-05-11T13:41:29Z

spylon_kernel/scala_magic.py

+        """
        intp = self._get_scala_interpreter()
        self.kernel.log.debug(info['help_obj'])
        # Calling this twice produces different output


what the heck. Why does this produce different results??

That's one @mariusvniekerk will need to answer.

Basically it's pretty heavily assumed that this is used in the scala repl. So it is the same call for two tabs. It is kinda mad

So, press tab twice for more details. Nice.

REPLs. REPLs all the way down.

parente · 2017-05-11T14:21:20Z

@ericdill Ready for another look.

ericdill · 2017-05-11T14:26:02Z

spylon_kernel/scala_interpreter.py

+
+        Returns
+        -------
+        scala.tools.nsc.interpreter.PresentationCompilerCompleter


Ah alright well fair enough then. Made up object paths, yey

ericdill · 2017-05-11T14:28:08Z

FWIW this has my blessing

parente · 2017-05-11T15:27:19Z

Thanks for taking the time @ericdill.

More docstrings and naming fixes

f1001c8

* Document scala_kernel * Rename SparkInterpreter to ScalaInterpreter for consistency with the factory function and module name

parente added 2 commits May 10, 2017 21:36

Docstring cleanup in the scala_magic

988f9f3

Remove type hints from scala_interpreter

40f950b

All or nothing. Nothing for now.

parente changed the title ~~[WIP] General cleanup take #2~~ General cleanup take #2 May 11, 2017

parente mentioned this pull request May 11, 2017

Fix lost stdout/stderr in secondary threads #31

Merged

ericdill suggested changes May 11, 2017

View reviewed changes

parente added 2 commits May 11, 2017 10:19

Address code review comments

9a7acbe

Fix some metakernel API comments

9fc9cc8

ericdill approved these changes May 11, 2017

View reviewed changes

parente merged commit 262c12f into adtech-labs:master May 11, 2017

Conversation

parente commented May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ericdill left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parente commented May 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericdill commented May 11, 2017

Uh oh!

parente commented May 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

parente commented May 10, 2017 •

edited

Loading

codecov bot commented May 10, 2017 •

edited

Loading