7620: Start removing dependencies on requests

[sturmer]

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]

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

[villebro]

awesome @sturmer ! I will review and test today.

[villebro]

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]

Thanks Ville, I'll check that out today.

[sturmer]

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]

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

[villebro]

Same as above.

[sturmer]

Done.

[betodealmeida]

@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]

@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]

@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]

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]

Here: dpgaspar/Flask-AppBuilder#1040

[sturmer]

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]

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).

[villebro]

@sturmer fab 2.1.5 is now in master

[sturmer]

Just restarted the build. Fingers crossed :)

[sturmer]

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]

@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]

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]

@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]

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]

@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]

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

Started this issue to track blockers:
#7974

[villebro]