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

Make S3 dependencies optional #404

Merged
merged 10 commits into from Feb 6, 2019

Conversation

Projects
None yet
3 participants
@floscha
Copy link
Contributor

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

This comment has been minimized.

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.

Allow user to pass in desired feature return types (#372)
* 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")

This comment has been minimized.

@kmax12

kmax12 Feb 6, 2019

Member

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: RaRe-Technologies/smart_open#250

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

This comment has been minimized.

@floscha

floscha Feb 6, 2019

Author Contributor

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

This comment has been minimized.

Copy link
Member

kmax12 commented Feb 6, 2019

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

@floscha

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

kmax12 commented Feb 6, 2019

Looks good merging

@kmax12 kmax12 merged commit 1ae3990 into Featuretools:master Feb 6, 2019

2 of 3 checks passed

codecov/patch 95.34% of diff hit (target 95.99%)
Details
codecov/project 96.07% (+0.07%) compared to de2ff0e
Details
license/cla Contributor License Agreement is signed.
Details

@rwedge rwedge referenced this pull request Feb 15, 2019

Merged

v0.6.1 #436

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.