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 dask an optional dependency #357

Merged
merged 14 commits into from Nov 9, 2020
Merged

Make dask an optional dependency #357

merged 14 commits into from Nov 9, 2020

Conversation

tamargrey
Copy link
Contributor

@tamargrey tamargrey commented Nov 5, 2020

todo:

  • Fix whatever the issue is with coverage this adds
  • Handle conda install
  • Add tests with and without dask to circleci config??

@codecov
Copy link

codecov bot commented Nov 5, 2020

Codecov Report

Merging #357 (b0c79b0) into main (e4435e9) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #357   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           29        29           
  Lines         3320      3342   +22     
=========================================
+ Hits          3320      3342   +22     
Impacted Files Coverage Δ
woodwork/data_column.py 100.00% <100.00%> (ø)
woodwork/data_table.py 100.00% <100.00%> (ø)
woodwork/deserialize.py 100.00% <100.00%> (ø)
woodwork/serialize.py 100.00% <100.00%> (ø)
woodwork/tests/conftest.py 100.00% <100.00%> (ø)
woodwork/tests/data_column/conftest.py 100.00% <100.00%> (ø)
woodwork/tests/data_column/test_data_column.py 100.00% <100.00%> (ø)
woodwork/tests/data_table/test_datatable.py 100.00% <100.00%> (ø)
woodwork/tests/data_table/test_serialization.py 100.00% <100.00%> (ø)
woodwork/tests/test_utils.py 100.00% <100.00%> (ø)
... and 2 more

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 e4435e9...b0c79b0. Read the comment docs.

@tamargrey tamargrey changed the title Make dask an optional dependencie Make dask an optional dependency Nov 5, 2020
@tamargrey
Copy link
Contributor Author

New circleci test structure looks like (where the optional tests get both unit testing and lint testing, and the minimal tests just get the unit testing)
image

Copy link
Contributor

@thehomebrewnerd thehomebrewnerd left a comment

Choose a reason for hiding this comment

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

I think in the CircleCI config we may be running codecov block twice (starting with the when condition on line 86). If I'm reading this right, this block will run any time we are running python 3.6. We should only run this when we have the optional libraries installed and python 3.6. I'm not great with digesting config.yml files, so I might be wrong.

Maybe take a look at what featuretools does for comparison:
https://github.com/FeatureLabs/featuretools/blob/c51ef4e2944bb754de5696833b480b316916202a/.circleci/config.yml#L170-L173

Other than that, I only noticed a couple other relatively minor things.

woodwork/tests/data_table/test_datatable.py Outdated Show resolved Hide resolved
docs/source/guides/using_woodwork_with_dask.ipynb Outdated Show resolved Hide resolved
docs/source/install.ipynb Outdated Show resolved Hide resolved
woodwork/deserialize.py Outdated Show resolved Hide resolved
docs/source/install.ipynb Outdated Show resolved Hide resolved
docs/source/install.ipynb Outdated Show resolved Hide resolved
@gsheni
Copy link
Contributor

gsheni commented Nov 6, 2020

The reason for the quotes is because some shells (like zsh) use square brackets for pattern matching while other shells (like bash) don't. Adding quotes makes it work for all shells.

Copy link
Contributor

@gsheni gsheni left a comment

Choose a reason for hiding this comment

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

Minor things. Looks good overall.

.circleci/config.yml Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@gsheni gsheni left a comment

Choose a reason for hiding this comment

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

lgtm

@gsheni gsheni removed the request for review from thehomebrewnerd November 9, 2020 14:49
@gsheni gsheni dismissed thehomebrewnerd’s stale review November 9, 2020 14:52

Fixes have been addressed

@tamargrey tamargrey merged commit 671e33d into main Nov 9, 2020
@tamargrey tamargrey deleted the optional-dask branch November 9, 2020 14:55
@gsheni gsheni mentioned this pull request Nov 11, 2020
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.

Add import_or_none to optionally import Dask Make Dask an optional depedency
3 participants