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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up lacking codecov for __main__.py and add evalml info to docs #1293

Merged
merged 30 commits into from
Oct 21, 2020

Conversation

angela97lin
Copy link
Contributor

@angela97lin angela97lin commented Oct 9, 2020

Closes #796
Updated docs here: https://evalml.alteryx.com/en/796_cli_check/user_guide/utilities.html

  • Fixes broken CLI evalml info. Try pip installing evalml and running evalml info--it currently breaks. Then, try using pip install git+ssh://git@github.com/alteryx/evalml.git@796_cli_check to install in fresh environment, cli should work now :)
  • Removes lines of code because it doesn't affect functionality and were previously missed by codecov.
  • I think current coverage of evalml info to the logger output suffices, capsys and caplog don't play really well with each other 馃槵

@angela97lin angela97lin added this to the October 2020 milestone Oct 9, 2020
@angela97lin angela97lin self-assigned this Oct 9, 2020
@codecov
Copy link

codecov bot commented Oct 9, 2020

Codecov Report

Merging #1293 into main will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1293   +/-   ##
=======================================
  Coverage   99.94%   99.94%           
=======================================
  Files         213      213           
  Lines       13387    13385    -2     
=======================================
- Hits        13379    13378    -1     
+ Misses          8        7    -1     
Impacted Files Coverage 螖
evalml/__main__.py 100.00% <酶> (+9.09%) 猬嗭笍

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 55e1776...05344f6. Read the comment docs.

@@ -22,5 +22,8 @@
]
},
data_files=[('evalml/demos/data', ['evalml/demos/data/fraud_transactions.csv.tar.gz', 'evalml/demos/data/churn.csv']),
("evalml/tests/data", ['evalml/tests/data/tips.csv', 'evalml/tests/data/titanic.csv'])]
('evalml/tests/data', ['evalml/tests/data/tips.csv', 'evalml/tests/data/titanic.csv'])],
package_data = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to add core-requirements.txt to package_data so that it is included in the binary distribution and available via pip installation.

Copy link
Contributor

Choose a reason for hiding this comment

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

This would install the core-requirements.txt file at the site-packages level instead of site-packages/evalml . It seems kind of weird that we would read a file that is technically not in our package and it could also conflict with other files called core-requirements.txt.

I think I would prefer the hardcode-a-list-of-requirements approach I think you mentioned at stand-up the other day.

image

Also, I think we should check if the ../ syntax works on windows?

Copy link
Contributor

Choose a reason for hiding this comment

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

@angela97lin @freddyaboulton ah, I see what you're talking about. The current impl of the evalml info CLI happens to read in core-requirements.txt in order to do its thing.

I suggest we change the impl instead:

  • In cli_utils.py delete get_core_requirements()
  • And update print_deps():
def print_deps(dependencies):
    logger.info("\nINSTALLED VERSIONS")
    logger.info("------------------")
    installed_packages = get_installed_packages()
    packages_to_log = []
    for package in installed_packages:
        packages_to_log.append((x, installed_packages[x]))
        logger.info("{package}: {version}".format(package=package, version=installed_packages[package]))

Will that work?

Also. good point about the pathing! So I think that data_files and package_data are used during package creation, meaning they need to work on whatever platform we build source dist / wheels on. We currently do this on linux. So, we're ok for now :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dsherry @freddyaboulton

Hm, is this what we want though? By removing get_core_requirements() / just calling get_installed_packages(), we're getting all of the packages the user has installed, regardless of if it is an evalml dependency or not. I'd argue then that this is not much more useful than pip freeze. If there's no good workaround, I'm okay with just having a hardcoded list as Featuretools has. Thoughts?

@angela97lin angela97lin marked this pull request as ready for review October 13, 2020 14:48
Copy link
Contributor

@dsherry dsherry left a comment

Choose a reason for hiding this comment

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

@angela97lin thanks for looking at this and for adding that guide!

I left a suggestion on the impl. Basically, I wonder if instead of packaging core-requirements.txt, we can just change the impl to log out the entirety of pip freeze. Let's conclude that conversation one way or another, and then I'll happily approve.



if __name__ == '__main__':
cli()
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, so this isn't needed for click to work?

@@ -22,5 +22,8 @@
]
},
data_files=[('evalml/demos/data', ['evalml/demos/data/fraud_transactions.csv.tar.gz', 'evalml/demos/data/churn.csv']),
("evalml/tests/data", ['evalml/tests/data/tips.csv', 'evalml/tests/data/titanic.csv'])]
('evalml/tests/data', ['evalml/tests/data/tips.csv', 'evalml/tests/data/titanic.csv'])],
package_data = {
Copy link
Contributor

Choose a reason for hiding this comment

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

@angela97lin @freddyaboulton ah, I see what you're talking about. The current impl of the evalml info CLI happens to read in core-requirements.txt in order to do its thing.

I suggest we change the impl instead:

  • In cli_utils.py delete get_core_requirements()
  • And update print_deps():
def print_deps(dependencies):
    logger.info("\nINSTALLED VERSIONS")
    logger.info("------------------")
    installed_packages = get_installed_packages()
    packages_to_log = []
    for package in installed_packages:
        packages_to_log.append((x, installed_packages[x]))
        logger.info("{package}: {version}".format(package=package, version=installed_packages[package]))

Will that work?

Also. good point about the pathing! So I think that data_files and package_data are used during package creation, meaning they need to work on whatever platform we build source dist / wheels on. We currently do this on linux. So, we're ok for now :)

"## System Information\n",
"\n",
"EvalML provides a command-line interface (CLI) tool prints the version of EvalML and core dependencies installed, as well as some basic system information. To use this tool, just run `evalml info` in your shell or terminal. This could be useful for debugging purposes or tracking down any version-related issues."
]
Copy link
Contributor

@dsherry dsherry Oct 20, 2020

Choose a reason for hiding this comment

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

@angela97lin I think there's a bug somewhere -- the output shows a stack trace in the docs

Screen Shot 2020-10-20 at 12 20 30 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I was testing out the suggestion you made. Reverted to working state here (https://evalml.alteryx.com/en/796_cli_check/user_guide/utilities.html) until we discuss.

Copy link
Contributor

@dsherry dsherry left a comment

Choose a reason for hiding this comment

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

Discussed with @angela97lin . Plan:

  • Merge this PR, since it adds missing code coverage
  • File issue to track removing core-requirements.txt from our package, and update the env CLI to list all installed packages (i.e. pip freeze)

@angela97lin
Copy link
Contributor Author

@dsherry Filed #1328, so this should be good to merge!

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/ensure test coverage for CLI commands
3 participants