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

Moved requirements_parser to core-requirements.txt #673

Merged
merged 3 commits into from Apr 17, 2020

Conversation

christopherbunn
Copy link
Contributor

@christopherbunn christopherbunn commented Apr 17, 2020

Fixed an issue where requirements_parser is not installed as a core dependency.

@christopherbunn christopherbunn requested a review from dsherry Apr 17, 2020
@dsherry dsherry added the bug Issues tracking problems with existing features. label Apr 17, 2020
Copy link
Collaborator

@dsherry dsherry left a comment

Super fast, thank you!

Backstory: I added requirements-parser as a dep for some test code a while back, and added it to test-requirements.txt. Then #300 added it as a core requirement but we forgot to get it into core-requirements.txt, and our test env installs both so there'd be no way to know that from the checkin tests.

I wonder what, if anything, we can do to avoid this in the future. Perhaps we need a smoke test which just installs core-requirements.txt and then tries to do a limited set of stuff. Or perhaps simply having the perf tests serves as that smoke test. Or, we don't need to do anything, because the chances of this situation happening again are low, i.e. forgetting to move an existing test req to core reqs.

In any case, thanks for getting this PR up!

@dsherry dsherry added this to the April 2020 milestone Apr 17, 2020
@dsherry
Copy link
Collaborator

dsherry commented Apr 17, 2020

@christopherbunn please move this to Review/QA

@christopherbunn christopherbunn force-pushed the cb_move_requirements_parser branch from ec5d975 to c7d3f00 Compare Apr 17, 2020
@codecov
Copy link

codecov bot commented Apr 17, 2020

Codecov Report

Merging #673 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #673   +/-   ##
=======================================
  Coverage   99.06%   99.06%           
=======================================
  Files         139      139           
  Lines        4907     4907           
=======================================
  Hits         4861     4861           
  Misses         46       46           
Impacted Files Coverage Δ
evalml/tests/utils_tests/test_cli_utils.py 100.00% <100.00%> (ø)

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 229b8e0...1b08653. Read the comment docs.

@christopherbunn christopherbunn force-pushed the cb_move_requirements_parser branch from c7d3f00 to 1b08653 Compare Apr 17, 2020
@christopherbunn christopherbunn merged commit 6fd092d into master Apr 17, 2020
2 checks passed
@dsherry dsherry deleted the cb_move_requirements_parser branch Oct 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues tracking problems with existing features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants