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

Add unit tests with minimum dependencies #2277

Merged
merged 46 commits into from May 20, 2021
Merged

Conversation

gsheni
Copy link
Contributor

@gsheni gsheni commented May 14, 2021

  • Add unit tests against minimum dependencies for python 3.7 on PRs (when requirements change) and to run on main.

@codecov
Copy link

codecov bot commented May 14, 2021

Codecov Report

Merging #2277 (91f9997) into main (a66ea32) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2277   +/-   ##
=======================================
  Coverage   100.0%   100.0%           
=======================================
  Files         280      280           
  Lines       24396    24396           
=======================================
  Hits        24373    24373           
  Misses         23       23           

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 a66ea32...91f9997. Read the comment docs.

@gsheni gsheni marked this pull request as ready for review May 14, 2021 23:00
Copy link
Contributor

@ParthivNaresh ParthivNaresh left a comment

Choose a reason for hiding this comment

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

Looks good to me, nice catch on upgrading numpy to fix the shap issue!

.github/workflows/linux_unit_tests_with_minimum_deps.yml Outdated Show resolved Hide resolved
.github/workflows/linux_unit_tests_with_minimum_deps.yml Outdated Show resolved Hide resolved
.github/workflows/linux_unit_tests_with_minimum_deps.yml Outdated Show resolved Hide resolved
.github/workflows/linux_unit_tests_with_minimum_deps.yml Outdated Show resolved Hide resolved
dask==2.12.0
distributed==2.12.0
Copy link
Contributor

Choose a reason for hiding this comment

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

How come we need to add this package? Should be pulled in by featuretools no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I had to add this is the following pip warning/error (which I saw after installing minimum_test_requirements.txt):

distributed 2021.5.0 has requirement dask==2021.05.0, but you'll have dask 2.12.0 which is incompatible.

Copy link
Contributor Author

@gsheni gsheni May 20, 2021

Choose a reason for hiding this comment

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

actually disregard that, I think the unit tests still pass on Linux without putting distributed.

Yes, ideally it should be pulled by featuretools (but my local Mac wasn't doing it).

Copy link
Contributor

@freddyaboulton freddyaboulton left a comment

Choose a reason for hiding this comment

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

@gsheni This looks great! Thanks for splitting up the tests into core and non-core too. I have a couple of questions about why we had to add/bump dependencies before I approve.

pytest==4.4.1
pytest-xdist==1.26.1
pytest-cov==2.6.1
wheel>=0.33.1
Copy link
Contributor

Choose a reason for hiding this comment

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

How come we need to add wheel and setuptools and bump pytest dependencies?

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 needed to add wheel and setuptools to make sure that numpy properly installed.
In addition, if you don't have wheel installed it takes longer to install some packages from PyPI (it does python setup.py install for each requirement). So installing wheel first fixes this.

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 bumped pytest because the old pytest had lower requirements that conflicted with requirements I was bumping.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it!

Copy link
Contributor

@freddyaboulton freddyaboulton left a comment

Choose a reason for hiding this comment

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

Thank you @gsheni !!

@gsheni gsheni merged commit f5ce75b into main May 20, 2021
@gsheni gsheni deleted the add_unit_tests_with_min_deps branch May 20, 2021 20:06
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.

None yet

4 participants