Skip to content

Conversation

@mistercrunch
Copy link
Member

Looking at the dependency tree for license related questions, I noticed
that tableschema had a huge tree, and only people running Hive really
need it. Making this as well as pyhive and thrift optional.

@codecov-io
Copy link

codecov-io commented Jan 16, 2019

Codecov Report

Merging #6696 into master will decrease coverage by 0.01%.
The diff coverage is 56.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6696      +/-   ##
==========================================
- Coverage   56.14%   56.12%   -0.02%     
==========================================
  Files         521      521              
  Lines       23179    23173       -6     
  Branches     2769     2769              
==========================================
- Hits        13014    13006       -8     
- Misses       9756     9758       +2     
  Partials      409      409
Impacted Files Coverage Δ
superset/db_engines/hive.py 0% <0%> (ø) ⬆️
superset/db_engine_specs.py 54.08% <0%> (-0.21%) ⬇️
superset/connectors/sqla/views.py 63.06% <0%> (-0.33%) ⬇️
superset/utils/core.py 88.4% <100%> (-0.05%) ⬇️
superset/dataframe.py 94.64% <100%> (-0.05%) ⬇️
superset/viz.py 71.66% <100%> (-0.02%) ⬇️
superset/connectors/base/models.py 90.37% <100%> (-0.06%) ⬇️

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 057c43c...f74b6d0. Read the comment docs.

Looking at the dependency tree for license related questions, I noticed
that tableschema had a huge tree, and only people running Hive really
need it. Making this as well as pyhive and thrift optional.

Also bumping some python dependencies
Copy link
Member

@john-bodley john-bodley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only comment is whether these optional dependencies should reside in the requirements.txt file. I can see the merit in having them there as one could argue it represents the frozen state of the union of all viable configurations.

I speculate we may need to add a requirements.in file to ensure that these are capture if someone wanted to regenerate the requirements.txt file in the future (see this for context).

@mistercrunch
Copy link
Member Author

Good call. I just didn't think about running pip-compile. I ran it just now and it removes the deps from requirements.txt which I think is what we want.

Now given that the extra_requires is optional, we could just be more prescriptive and pin things in that section of setup.py if needed/appropriate. Being prescriptive inside something optional is very different than being prescriptive in something that is forced/required.

@john-bodley
Copy link
Member

john-bodley commented Jan 19, 2019

@mistercrunch I think the following would work if the desire is to pin the optional packages (and their dependencies) especially for testing purposes:

> cat requirements.in
-e .[gsheets,hive,presto]
> pip-compile --output-file requirements.txt requirements.in
...

@mistercrunch mistercrunch merged commit f742b98 into apache:master Jan 19, 2019
@mistercrunch mistercrunch deleted the opt_deps branch January 19, 2019 22:27
This file documents any backwards-incompatible changes in Superset and
assists people when migrating to a new version.

## Superset 0.32.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mistercrunch this PR got into 0.31 so this should be in the 0.31 section. Also I think we should call out the specific dependencies that were removed, we were impacted by removing flower would be good to list that out.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I assumed this would hit 0.32... let me change that then...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here: #7117

@michellethomas michellethomas added the risk:breaking-change Issues or PRs that will introduce breaking changes label Mar 25, 2019
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 First shipped in 0.34.0 labels Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels risk:breaking-change Issues or PRs that will introduce breaking changes 🚢 0.34.0 First shipped in 0.34.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants