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

7620: Start removing dependencies on requests #7643

Merged
merged 24 commits into from Aug 2, 2019

Conversation

@sturmer
Copy link
Contributor

@sturmer sturmer commented Jun 4, 2019

CATEGORY

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

SUMMARY

Make dependency on requests and pydruid connector optional. Note: the first commit is @villebro 's work.

TEST PLAN

It might require some @unittest.skipUnless, I will check the result of the Travis tests first.

ADDITIONAL INFORMATION

Fixes #7620.

  • Has associated issue: 7620.
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

REVIEWERS

@mistercrunch , @villebro

@sturmer
Copy link
Contributor Author

@sturmer sturmer commented Jun 18, 2019

@mistercrunch @villebro Do you want to have a look? Unless I missed something this should be it.

@sturmer sturmer changed the title [WIP] 7620: Start removing dependencies on requests 7620: Start removing dependencies on requests Jun 18, 2019
@pull-request-size pull-request-size bot added size/M and removed size/S labels Jun 18, 2019
@villebro
Copy link
Member

@villebro villebro commented Jun 18, 2019

awesome @sturmer ! I will review and test today.

@villebro
Copy link
Member

@villebro villebro commented Jun 18, 2019

Two problems:

  1. When I tried to load examples, I got an exception on superset/connectors/druid/models.py, which has an import on requests. This needs to be wrapped in a try/catch.
  2. flask-appbuilder has a dependency on prison, which in turn has a dependency on requests. I believe this dependency was introduced recently.

The first observation is easy to fix, but the second is more difficult.. @dpgaspar @betodealmeida do you think requests can be replaced with urllib on FAB/Prison?

@sturmer
Copy link
Contributor Author

@sturmer sturmer commented Jun 19, 2019

Thanks Ville, I'll check that out today.

@sturmer
Copy link
Contributor Author

@sturmer sturmer commented Jun 19, 2019

I've had a look at Beto's fork of prison and I can't see any usage of requests 🤔
I've opened a PR proposing its removal, waiting for comments!

(In the meanwhile, I'll try to fix the dependencies in setup.py and see where I get.)

@sturmer
Copy link
Contributor Author

@sturmer sturmer commented Jun 19, 2019

Note: This last commit won't work until prison stops depending on requests.

superset/connectors/druid/models.py Outdated Show resolved Hide resolved
it seems it is not installed on your system. Aborting...
""")
from sys import exit
exit(1)
Copy link
Member

@villebro villebro Jun 19, 2019

Same as above.

Copy link
Contributor Author

@sturmer sturmer Jun 21, 2019

Done.

@betodealmeida
Copy link
Member

@betodealmeida betodealmeida commented Jun 19, 2019

@villebro

flask-appbuilder has a dependency on prison, which in turn has a dependency on requests. I believe this dependency was introduced recently.

I just published prison==0.1.2, without the dependency, thanks to @sturmer! :)

@sturmer
Copy link
Contributor Author

@sturmer sturmer commented Jun 20, 2019

@villebro I've started messing around with the try-caught imports and it seems the business gets quite deep. For instance, if I don't correctly from pydruid.utils.having import Having, then I can't declare DimSelector. If I don't declare DimSelector, then I can't use _get_having_obj(self, col, op, eq) in models.py. And of course it starts getting deeper than that -- I haven't followed it deep enough but I suspect the whole Python file becomes useless.

Would it make sense to remove completely the druid-related code from the db upgrade and load_examples commands (whatever that means)? Or you just suggest to pass every time we can't correctly declare a symbol for lack of imports?

@villebro
Copy link
Member

@villebro villebro commented Jun 20, 2019

@sturmer I agree, it's not trivial and there are several options, all with pros and cons. Let me digest this for a while and get back with a proposal.

@villebro
Copy link
Member

@villebro villebro commented Jun 20, 2019

Btw, something that can be done now is a PR for https://github.com/dpgaspar/Flask-AppBuilder , i.e. bumping prison and removing requests there also. FYI @dpgaspar

@mistercrunch
Copy link
Member

@mistercrunch mistercrunch commented Jun 21, 2019

Here: dpgaspar/Flask-AppBuilder#1040

@sturmer
Copy link
Contributor Author

@sturmer sturmer commented Jun 21, 2019

I haven't followed it deep enough but I suspect the whole Python file becomes useless.

Well in the end it didn't go so deep after all. I've managed to have superset load_examples and superset db upgrade end successfully (at least without emitting errors), now waiting for the results from Travis.

@sturmer
Copy link
Contributor Author

@sturmer sturmer commented Jun 21, 2019

Hm, if I understand correctly this build won't work until the bump of prison is merged into fab (and the new resulting fab version is specified as a dependency).
image

@villebro
Copy link
Member

@villebro villebro commented Jun 21, 2019

@sturmer fab 2.1.5 is now in master

@sturmer
Copy link
Contributor Author

@sturmer sturmer commented Jun 21, 2019

Just restarted the build. Fingers crossed :)

@sturmer sturmer force-pushed the 7620-make-requests-optional-dep branch from 5c5b46e to e996f77 Jun 25, 2019
@sturmer
Copy link
Contributor Author

@sturmer sturmer commented Jun 25, 2019

It's time to heed the advice by Maxime and use the ideas from here to skip tests that depend on PyDruid & co. Will work on that tomorrow.

@villebro
Copy link
Member

@villebro villebro commented Jun 25, 2019

@sturmer I have a few ideas of how to do the conditional importing in a simpler way. Should I make a PR to your branch? Optionally if I can have write access to the branch I could contribute commits, making it easier to collaborate.

@mistercrunch
Copy link
Member

@mistercrunch mistercrunch commented Jun 26, 2019

Sorry about the conflicts, we just merged a large PR running black on the whole codebase. It should be easy to apply black on your branch and rebase. Let us know if it's not straightforward.

@sturmer
Copy link
Contributor Author

@sturmer sturmer commented Jun 26, 2019

@villebro Sure, I thought you could push to this branch as a project maintainer. If you aren't, do you happen to know how I can authorize you? (I've googled it briefly and couldn't find anything relevant.)

@mistercrunch No worries, I'll solve the conflicts.

@sturmer sturmer force-pushed the 7620-make-requests-optional-dep branch from 9eeee06 to ab87fc7 Jun 26, 2019
@sturmer sturmer force-pushed the 7620-make-requests-optional-dep branch from d615c28 to c79031c Aug 2, 2019
@sturmer
Copy link
Contributor Author

@sturmer sturmer commented Aug 2, 2019

I was also thinking: Maybe we could remove the entries in requirements.txt which are there # via requests, right?

Log: solved merge conflicts, maybe this CI run will be green 🤞

@sturmer
Copy link
Contributor Author

@sturmer sturmer commented Aug 2, 2019

@villebro @mistercrunch @betodealmeida @john-bodley CI green! It is now or never! (Just kidding, take your time -- but if anyone feels like reviewing I'd appreciate feedback. Quoting Ville:

The try/except additions in connectors/druid/models.py still feel slightly messy, but I feel they should be acceptable given the tradeoff of being able to drop problematic licenses. This is also something I feel we can get improve later if needed. Here I think @mistercrunch @john-bodley @betodealmeida could weigh in with comments on whether or not the current solution is appropriate.

)

@mistercrunch mistercrunch merged commit e23920b into apache:master Aug 2, 2019
1 check passed
@mistercrunch
Copy link
Member

@mistercrunch mistercrunch commented Aug 2, 2019

Nice! Merging while the button is green, we really need to make some progress towards releases!

Started this issue to track blockers:
#7974

Copy link
Member

@villebro villebro left a comment

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants