Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Make pyarrow and msgpack optional #8218

Closed

Conversation

bkyryliuk
Copy link
Member

@bkyryliuk bkyryliuk commented Sep 12, 2019

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

Makes msgpack and pyarrow dependency optional.
Follow up to the #8069

Some context, we use bazel @ dropbox and bazelyfing pyarrow is quite challenging due to the a bit messy build there e.g. shelling out to cmake.
It would be great to keep not only functionality but also pip dependency optional as well. Please let me know what you think.

REVIEWERS

@bkyryliuk bkyryliuk force-pushed the bogdan/make_pyarrow_optional branch from 1f2169a to 87591f7 Compare September 12, 2019 00:11
@bkyryliuk bkyryliuk changed the title Bogdan/make pyarrow optional Make pyarrow and msgpack optional Sep 12, 2019
@pull-request-size pull-request-size bot added size/S and removed size/M labels Sep 12, 2019
@bkyryliuk bkyryliuk force-pushed the bogdan/make_pyarrow_optional branch from 54b4d1a to 8a14030 Compare September 12, 2019 17:54
@robdiciuccio
Copy link
Member

Based on the performance gains of msgpack + pyarrow over raw JSON, it would be ideal to standardize on this serialization approach going forward, pending community testing. I'm not familiar with bazel, but can you expand a bit on the difficulties there?

@bkyryliuk bkyryliuk changed the title Make pyarrow and msgpack optional [WIP] Make pyarrow and msgpack optional Sep 12, 2019
@bkyryliuk
Copy link
Member Author

bkyryliuk commented Sep 12, 2019

@robdiciuccio pyarrow has only binary distributions for the 0.14.1 and https://github.com/apache/arrow/blob/master/python/setup.py#L284 shells out to the cmake during install. There is a workaround via cloning, defining bazel rules and building it internally.

I am all in for the standardization, my only preference is to have it optional during the community
testing phase.

@bkyryliuk bkyryliuk force-pushed the bogdan/make_pyarrow_optional branch from 73f63d8 to 54fbef0 Compare September 12, 2019 22:22
@bkyryliuk bkyryliuk force-pushed the bogdan/make_pyarrow_optional branch 2 times, most recently from 0b5f6d3 to 605692d Compare November 11, 2019 22:39
Move pyarrow back for unit tests

Disable msgpack by default

Fix tests
@bkyryliuk bkyryliuk force-pushed the bogdan/make_pyarrow_optional branch from 605692d to ee98f94 Compare November 12, 2019 01:51
@codecov-io
Copy link

codecov-io commented Nov 12, 2019

Codecov Report

Merging #8218 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #8218      +/-   ##
=========================================
+ Coverage    66.8%   66.8%   +<.01%     
=========================================
  Files         449     449              
  Lines       22703   22705       +2     
  Branches     2366    2366              
=========================================
+ Hits        15166   15168       +2     
  Misses       7399    7399              
  Partials      138     138
Impacted Files Coverage Δ
superset/sql_lab.py 78.03% <100%> (+0.1%) ⬆️
superset/config.py 90% <100%> (ø) ⬆️
superset/views/core.py 71.98% <100%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62476c5...ee98f94. Read the comment docs.

@bkyryliuk bkyryliuk closed this Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants