ZEPPELIN-1423 - Allow users to specify pre/post-execute hooks for interpreters

[agoodm]

What is this PR for?

See #1439. This is a second attempt based on prior feedback, particularly from @Leemoonsoo who advised that it would be better to contain all of the code inside of the Interpreter Process. Aside from this, the major changes since the previous PR are:

• The terminology "hooks" is used instead of "callbacks". Since the information being passed to the interpreter isn't actually a "callback", strictly speaking, the more general term "hook" is probably a better choice. All variable / class names were updated correspondingly.
• The registry is now keyed by class name rather than the replName specified by the magic on top of the paragraph. The interface provided by ZeppelinContext allows for a replName to be converted to className through an internal mapping.
• Two new event codes, PRE_EXEC_DEV and POST_EXEC_DEV have been added. This is primarily intended to separate use cases for the interpreter maintainers (via subclasses of Interpreter) and the notebook users (via the ZeppelinContext class), as otherwise user attempts at registering a hook would overwrite that set by the interpreter maintainer.
• Global scope for hook registration is supported for the developer use cases.

What type of PR is it?

New Feature

What is the Jira issue?

ZEPPELIN-1423

How should this be tested?

In a new note, add the following lines of code to a paragraph:

%pyspark
z.registerHook("post_exec", "print 'This code should be executed before the paragraph code!'")
z.registerHook("pre_exec", "print 'This code should be executed after the paragraph code!'")

Then run any other paragraph in the note containing some other code, eg

%pyspark
print "This code should be entered into the paragraph by the user!"

The output should be:

This code should be executed before the paragraph code!
This code should be entered into the paragraph by the user!
This code should be executed after the paragraph code!

You should also test out the other two methods (getCallback() and unregisterCallback()) specified in ZeppelinContext.java.

One final caveat that should be mentioned: If there are errors in the code you specify for a pre-execute event, it will render the interpreter useless since the current implementation prepends the the code specified in pre_exec directly to the paragraph entered code before calling interpret(). The current workaround for this would be to either restart the interpreter group or call unregisterHook() via a different REPL within the interpreter group (eg, z.unregisterHook("pre_exec", "pyspark") from the spark interpreter). I would appreciate if anyone here would be willing to share any better approaches here.

Questions:

• Does the licenses files need update? No
• Is there breaking changes for older versions? No
• Does this needs documentation? Yes

[agoodm]

@Leemoonsoo please review, thanks.

[Leemoonsoo]

@agoodm Thanks for the contribution. I've left some comments. Please take a look and let me know how you think.

[Leemoonsoo]

InterpreterHookRegistry is directly passed to ZeppelinContext on it's constructor. And ZeppelinContext is exposed to user directly. So currently Interpreter.registerHook(),Interpreter.unregiserHook(),Interpreter.getHook() are accessed from only RemoteInterpreterServer. But RemoteInterpreterServer also can get directly access InterpreterHookRegistry from InterpreterGroup. In this way, i think the feature can be implemented without modification of Interpreter.java. What do you think?

[agoodm]

I kind of see your point, moving the convenience methods from Interpreter.java to InterpreterGroup.java would save the need for defining wrappers in LazyOpenInterpreter.java. However this would also mean that the interpreter className must always be specified. This means for example that this line:

String cmdDev = interpreter.getHook(noteId, HookType.PRE_EXEC_DEV);

Would be changed to

String className = interpreter.getClassName();
String cmdDev = interpreter.getInterpreterGroup().getHook(noteId, className, HookType.PRE_EXEC_DEV);

Additionally while these methods are only currently called through RemoteInterpreterServer, I will have use cases for directly calling them in the Python Interpreter subclasses (eg, PySparkInterpreter and PythonInterpreter) for my upcoming matplotlib integration work. Given this, I felt that the current syntax would be clearer.

[Leemoonsoo]

Since Zeppelin allows use alias to select interpreter (see, https://issues.apache.org/jira/browse/ZEPPELIN-1012), this method will not only return "spark", "pyspark", etc but also any arbitrary alias. So, this value can be the key not exists in interpreterClassMap.

[agoodm]

I forgot about this, this definitely sounds like it would be a problem. I was struggling to figure out ways to automatically detect the current interpreter class through ZeppelinContext since I thought that would be rather convenient for users, but I couldn't come up with better ideas at the time.

I think this problem can be partially addressed by passing in the interpreter class name through the current InterpreterContext instance as that could solve the problem for automatically detecting the current interpreter that's in use. However this still wouldn't allow users to manually specify aliases for another interpreter that isn't keyed into the internal mapping. Let me know what you think of this solution and if you have any other ideas.

[agoodm]

@Leemoonsoo Thanks for the review, I have provided some responses.

[agoodm]

@Leemoonsoo I have addressed your second comment, let me know if this looks good to merge into master.

[Leemoonsoo]

Apologies for late response. Changes LGTM.
Last CI failure is irrelevant to this change.
Merge if there're no more discussion.

Thanks @agoodm for making step forward to ZEPPELIN-1344!