[Zeppelin-630] Introduce new way of dependency loading to intepreter #673
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Manage repository information in `conf/interpreter.json` * Front-end modification to manage repository list * Add RestApi for adding/deleting repository * Fix tests
Contributor
|
This is really nice feature and a lot of helpful for users. |
Contributor
|
There are some bugs.
|
Contributor
There was a problem hiding this comment.
Why do you use Console here?
Isnt it LOG better? so you can print log like LOG.warning('deprecated blablalba')?
Member
Author
There was a problem hiding this comment.
I used Console to print deprecated message in paragraph output through scala compiler.
Can we keep it as it is?
Member
There was a problem hiding this comment.
I think printing message in the notebook is using Console is better in this case.
Contributor
There was a problem hiding this comment.
agreed, i didnt know what it was doing so thats why i asked :)
Member
|
Awesome |
Member
Author
|
@astroshim, @anthonycorbacho Thank you for the review. I pushed commits to address your comment. Please take a look into this again. |
Member
|
Would it be possible to define dependency for a notebook? I suspect some users are currently having a %dep paragraph at the top of the notebook to bring down dependencies |
Member
Author
|
@felixcheung yes load dependency using %dep still works, and I simply added the deprecated message on the use of |
Contributor
|
Tested and working well. 👍 |
Contributor
There was a problem hiding this comment.
Input inline tags needs to be closed
minahlee
force-pushed
the
ZEPPELIN-630
branch
3 times, most recently
from
dc5f4be to
387e21e
Compare
Member
Author
|
Every comments has been addressed and test/docs are updated. Ready for review |
minahlee
changed the title
bin/interpreter.sh
Outdated
Member
There was a problem hiding this comment.
local repo would be .m2? don't we need to recursively add all jars in subdirectory then?
Member
Author
There was a problem hiding this comment.
Once user loads the libraries, Zeppelin downloads them to ZEPPELIN_HOME/local-repo first, and copy only files to ZEPPELIN_HOME/local-repo/{interpreterId}. So every file under this directory will be file, not directory. Maybe naming is confusing. Will it help to change name of this variable to ${LOCAL_REPO_INTERPRETER_DIR}?
Member
There was a problem hiding this comment.
ah got it.
It's a bit confusing, as "local repository" is frequently used with Maven https://maven.apache.org/guides/introduction/introduction-to-repositories.html
"There are strictly only two types of repositories: local and remote. The local repository refers to a copy on your own installation that is a cache of the remote downloads, and also contains the temporary build artifacts that you have not yet released."
minahlee
force-pushed
the
ZEPPELIN-630
branch
2 times, most recently
from
c854445 to
545c173
Compare
…into ZEPPELIN-630 Conflicts: zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreter.java zeppelin-zengine/src/main/java/org/apache/zeppelin/interpreter/InterpreterFactory.java
Member
|
LGTM and merge if there're no more discussions. |
Contributor
|
@minahlee thanks for this awesome feature! It seems to me that it does not save these settings on restarting zeppelin, however. Can you confirm that this is the expected behaviour? |
Contributor
|
When I tried to save it, it gave me this:
It didn't seem to save, though. |
Member
Author
|
@FRosner Thank you for reporting it. Seems like you have misspelled |
Contributor
|
Thanks for pointing this out! I will try to reproduce it in the next days and provide you with the logs. I had to use Internet Explorer at this time, so this might be the reason. Can I add artifacts that are local jars as well? So like with %dep z.load("/path/to/my.jar")? Thanks for your response :) |
Member
Author
|
@FRosner Yes you can also add local jars :) but you don't need to use %dep z.load("/path/to/my.jar") You can do this just by adding one more artifact( |
Contributor
|
Understood @minahlee thank you. I will try it tomorrow and also reproduce the missing error message for you. Thanks again! |
Contributor
|
Ok everything works as expected if I don't spell my dependencies wrong. However, if I do, I still get this empty error box. The logs show that the dependency does not exist so this seems to make sense. The browser I was using (new machine so I didn't have time to install a proper one), was Internet Explorer 11.0.9600.18124. |
Contributor
|
@minahlee when I try to add a local jar that I added via |
Member
Author
|
FRosner I found the way to reproduce this bug. This happens when you have empty artifact in the rows other than the last row as below:
I got the hint from Please let me know it's same as your case and try if it works when you have no empty artifact. Meanwhile I will try to create a patch for this bug sooner or later. |
Contributor
|
Understood @minahlee. I did not have an empty row except the last one (with the + on the right) I think. But because there is a +, I was not able to remove it. Maybe this is the reason. Do you ignore the last row if it is empty? |
Member
Author
asfgit
pushed a commit
that referenced
this pull request
Feb 20, 2016
…fact is empty ### What is this PR for? This PR returns empty list of dependency files instead of throwing error when dependency artifact input form in interpreter setting is empty. ### What type of PR is it? Bug Fix ### Is there a relevant Jira issue? No. The bug is reported through comment in #673 ### Questions: * Does the licenses files need update? No * Is there breaking changes for older versions? No * Does this needs documentation? No Author: Mina Lee <minalee@nflabs.com> Closes #716 from minahlee/fix/emptyArtifact and squashes the following commits: 22d5344 [Mina Lee] Return empty file list instead of throwing error when dependency artifact is empty
Contributor
|
@minahlee Is there any follow up change about this ? I didn't find the repository management section in the interpreter page. |
Member
Author
|
@zjffdu If you click gear button located top right side, it will toggle repository management page |
Contributor
|
Thanks @minahlee, it works. But any reason to make it hidden by default ? Because it is not intuitive for me that I can click the gear button to display it. |
|
I'm finding that using %dep interpreter no longer works for me on Zeppelin 0.6.0. Am I alone? e.g. Whereas this works: |
Contributor
|
Haven't tried @nihakue. Can you try to load a jar through z.load? Maybe it's some issue related to Spark packages + %dep? And please keep in mind that you need to start the %dep as the very first thing before running anything else after restarting the interpreter. |
|
Hey @FRosner, What do you mean by loading a jar through z.load? Sorry, but isn't that what this does: Or do you mean a local jar? I can give that a shot. Also, I am running the %dep interpreter first thing. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is this PR for?
With this PR user will be able to set external libraries to be loaded to specific interpreter.
Note that the scope of this PR is downloading libraries to local repository, not distributing them to other nodes. Only spark interpreter distributes loaded dependencies to worker nodes at the moment.
Here is a brief explanation how the code works.
ZEPPELIN_HOME/local-repoand copy them toZEPPELIN_HOME/local-repo/{interpreterId}ZEPPELIN_HOME/local-repo/{interpreterId}/*.jarare added to interpreter classpath when interpreter process startsWhat type of PR is it?
Improvement
Todos
Is there a relevant Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-630
And this PR will resolve ZEPPELIN-194 ZEPPELIN-381 ZEPPELIN-609
How should this be tested?
Add repository(in interpreter menu, click gear button placed top right side)
Set dependency in spark interpreter(click edit button of spark interpreter setting)
Download example csv file
run below code in paragraph
Screenshots (if appropriate)
Questions:
dependenciesobject should be added to request payload.