Skip to content

Make S3 dependencies optional #404

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

Merged
merged 10 commits into from
Feb 6, 2019
Merged

Make S3 dependencies optional #404

merged 10 commits into from
Feb 6, 2019

Conversation

floscha
Copy link
Contributor

@floscha floscha commented Feb 5, 2019

Based on the discussion in #340, this PR makes all S3-related dependencies optional, considering that S3-usage is presumably limited to a smaller range of users. However, the dependencies were moved to test-requirements.txt so that unit tests still run.

One corner case I left out is S3 buckets which are not supported right now. Adding such support would (IMHO) require adding some regular expressions which would unnecessarily clutter the code. If preferred, I can add those nonetheless.

@codecov
Copy link

codecov bot commented Feb 5, 2019

Codecov Report

Merging #404 into master will increase coverage by 0.07%.
The diff coverage is 95.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #404      +/-   ##
==========================================
+ Coverage   95.99%   96.07%   +0.07%     
==========================================
  Files          92       92              
  Lines        7996     8024      +28     
==========================================
+ Hits         7676     7709      +33     
+ Misses        320      315       -5
Impacted Files Coverage Δ
featuretools/synthesis/deep_feature_synthesis.py 96.57% <100%> (+2%) ⬆️
...ols/tests/dfs_tests/test_deep_feature_synthesis.py 98.49% <100%> (+0.12%) ⬆️
featuretools/synthesis/dfs.py 100% <100%> (ø) ⬆️
featuretools/primitives/install.py 89.07% <50%> (-1.45%) ⬇️

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 de2ff0e...cc36718. Read the comment docs.

* change method var variable_types to prevent overloading

* add and pass allowed_variable_types var to _run_dfs and _build_forward_features

* allowed_variable_types can be passed to dfs

* correct docstrings for allowed_variable_types in dfs and deep_feature_synthesis.build_features

* fix linting issues.

* fix linting errors

* fix another linting error

* fix F632 linter error

* add test. rename build_features return_variable_types arg.

* remove redundant return_variable_types attr from deep_feature_synthesis._run_dfs()

* add datetime tests. minor formatting change.

* remove bad line break

* Update deep_feature_synthesis.py
import s3fs
from botocore.exceptions import NoCredentialsError
except ImportError:
raise ImportError("The s3fs library is required to handle s3 files")
Copy link
Contributor

Choose a reason for hiding this comment

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

s3fs is actually only used as a fallback since smart_open cannot handle the anonymous clients which leads to the NoCredentialsError case. smart open can open s3 files in other cases.

see this issue: piskvorky/smart_open#250

perhaps move this try/except into the NoCredentialsError block which is the only place s3fs is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I was assuming that smart_open also uses s3fs under the hood. I've now placed the s3fs import as you suggested.

@kmax12
Copy link
Contributor

kmax12 commented Feb 6, 2019

do we need to add the s3fs check to the load_retail or load_flight functions as well?

@floscha
Copy link
Contributor Author

floscha commented Feb 6, 2019

Both load_retail and load_flight work without having s3fs installed. However, I noticed that smart_open crashes when boto3 isn't installed (which already happens when calling import featuretools), so I added it back to requirements.txt with 401f4d0.

@kmax12
Copy link
Contributor

kmax12 commented Feb 6, 2019

Looks good merging

@kmax12 kmax12 merged commit 1ae3990 into alteryx:master Feb 6, 2019
@rwedge rwedge mentioned this pull request Feb 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants