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

Fix 'pip install .' #4856

Merged
merged 2 commits into from
Apr 23, 2018
Merged

Fix 'pip install .' #4856

merged 2 commits into from
Apr 23, 2018

Conversation

mistercrunch
Copy link
Member

Fix error :

flask-appbuilder 1.10.0 has requirement Flask-SQLAlchemy==2.1,
but you'll have flask-sqlalchemy 2.3.2 which is incompatible.
botocore 1.10.5 has requirement python-dateutil<2.7.0,>=2.1, but you'll
have python-dateutil 2.7.2 which is incompatible.

Fix error :
> flask-appbuilder 1.10.0 has requirement Flask-SQLAlchemy==2.1,
> but you'll have flask-sqlalchemy 2.3.2 which is incompatible.
> botocore 1.10.5 has requirement python-dateutil<2.7.0,>=2.1, but you'll
> have python-dateutil 2.7.2 which is incompatible.
@codecov-io
Copy link

codecov-io commented Apr 20, 2018

Codecov Report

Merging #4856 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4856   +/-   ##
=======================================
  Coverage   76.96%   76.96%           
=======================================
  Files          44       44           
  Lines        8534     8534           
=======================================
  Hits         6568     6568           
  Misses       1966     1966

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 c92b56e...7c00091. Read the comment docs.

@john-bodley
Copy link
Member

@mistercrunch isn't the fix to resolve the version specified in the requirements.txt file? I'm also interested in learning why this hasn't been an issue for Travis CI.

@mistercrunch
Copy link
Member Author

Probably because of https://github.com/apache/incubator-superset/blob/master/tox.ini#L29, if you install the requirements.txt first and then run pip install . then it works.

For me if I try a simple pip install . from an empty virtualenv I get the error above. Note that setuptools is not aware of requirements.txt, only of what's in setup.py

@john-bodley
Copy link
Member

@mistercrunch I'll try to triage the problem seeing as I introduced the regression. If we remove the dependency we'll also need to update requirements.txt.

@john-bodley
Copy link
Member

@mistercrunch I tested the following in Python 2.7 and 3.6 and wasn't able to reproduce your issue:

virtualenv env
source env/bin/activate
pip install -r requirements.txt
pip install .

@mistercrunch
Copy link
Member Author

@john-bodley but the point is that it doesn't work if you don't pip install -r requirements.txt.

When you pip install superset from pypi, it only knows about what's in setup.py, and it doesn't know about the requirements.txt file (it's not even in the package). I'm pretty sure if I was to publish what is in master (before this PR) to pypi, it would break pip install superset.

@john-bodley
Copy link
Member

john-bodley commented Apr 23, 2018

@mistercrunch sorry from the title it mentioned pip install . and thus I thought it was a local install. Note it seems like we don't use flask-sqlalchemy, i.e, the following produces no hits:

 grep -R flask_sqlalchemy *

I think your change is fine if you also remove the flask-sqlalchemy==2.1 line from requirements.txt.

@mistercrunch
Copy link
Member Author

Yes what I was trying to get to is that pip install . is a local equivalent of how a pip install superset will behave once uploaded to PyPI and that it was broken.

flask-sqlalchemy is a FAB dep that somehow crawled its way into our setup.py probably at a point in time where it needed to be pinned and FAB had not pinned it yet. Now I'm unclear on whether it belongs in requirements.txt

@john-bodley
Copy link
Member

Per my last comment, I recommend removing the line from requirements.txt as it’s unused by Superset directly.

@mistercrunch mistercrunch merged commit 590e346 into master Apr 23, 2018
@mistercrunch mistercrunch deleted the fix_build branch April 23, 2018 15:16
michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
* Fix 'pip install .'

Fix error :
> flask-appbuilder 1.10.0 has requirement Flask-SQLAlchemy==2.1,
> but you'll have flask-sqlalchemy 2.3.2 which is incompatible.
> botocore 1.10.5 has requirement python-dateutil<2.7.0,>=2.1, but you'll
> have python-dateutil 2.7.2 which is incompatible.

* remove flask-sqlalchemy==2.1 from reqs.txt
timifasubaa pushed a commit to timifasubaa/incubator-superset that referenced this pull request May 31, 2018
* Fix 'pip install .'

Fix error :
> flask-appbuilder 1.10.0 has requirement Flask-SQLAlchemy==2.1,
> but you'll have flask-sqlalchemy 2.3.2 which is incompatible.
> botocore 1.10.5 has requirement python-dateutil<2.7.0,>=2.1, but you'll
> have python-dateutil 2.7.2 which is incompatible.

* remove flask-sqlalchemy==2.1 from reqs.txt
@gwbx gwbx mentioned this pull request Oct 2, 2018
3 tasks
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
* Fix 'pip install .'

Fix error :
> flask-appbuilder 1.10.0 has requirement Flask-SQLAlchemy==2.1,
> but you'll have flask-sqlalchemy 2.3.2 which is incompatible.
> botocore 1.10.5 has requirement python-dateutil<2.7.0,>=2.1, but you'll
> have python-dateutil 2.7.2 which is incompatible.

* remove flask-sqlalchemy==2.1 from reqs.txt
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.25.0 labels Feb 27, 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 🚢 0.25.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants