Skip to content
This repository has been archived by the owner on Sep 1, 2022. It is now read-only.

Remove requirement of SparkR #533

Merged
merged 3 commits into from
Mar 15, 2017
Merged

Remove requirement of SparkR #533

merged 3 commits into from
Mar 15, 2017

Conversation

wjsi
Copy link
Contributor

@wjsi wjsi commented Dec 11, 2016

As R users are not always using SparkR, it is better to activate the Spark DataFrame serializer only when SparkR is installed.

BTW, check_packages() in declarativewidgets/kernel-r/declarativewidgets/R/serializers.r always returns TRUE. It is also fixed in this PR.

@wjsi wjsi changed the title [WIP] Remove requirements for SparkR [WIP] Remove requirement of SparkR Dec 11, 2016
@poplav
Copy link
Contributor

poplav commented Dec 11, 2016

@siyuwj

That is a good point. I see a few things that can be done:

  • In load_serializers we should first check the serializer has it's dependencies via check_packages. This is a bit cleaner than the initial commit for this PR and is what the python side does here. We should also add logging that the package failed to load and the serializer was not inited correctly.
  • The tests directly use sparkR also, so those should be separated out and logged that sparkR type tests were skipped due to unmet dependencies.

@wjsi
Copy link
Contributor Author

wjsi commented Dec 12, 2016

@poplav I added check_packages in register_serializer. Spark tests are skipped when SparkR is not installed.

@poplav
Copy link
Contributor

poplav commented Dec 12, 2016

@siyuwj I think your changes look good, we are having issues with tests at the moment so bear with us regarding timing of merging.

@wjsi wjsi changed the title [WIP] Remove requirement of SparkR Remove requirement of SparkR Dec 13, 2016
@poplav
Copy link
Contributor

poplav commented Mar 14, 2017

@siyuwj Thanks again for your contribution. We recently finished a revamp of our testing/build process at #532. Can you rebase this with master and then we'll merge it!

@poplav poplav merged commit 0848a9e into jupyter:master Mar 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants