-
Notifications
You must be signed in to change notification settings - Fork 91
Add util methods to get system / environment info #300
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #300 +/- ##
=======================================
Coverage 98.97% 98.97%
=======================================
Files 136 136
Lines 4685 4685
=======================================
Hits 4637 4637
Misses 48 48 Continue to review full report at Codecov.
|
@angela97lin said: this is good to go, maybe missing tests. Next step: I'll review |
print_info() | ||
|
||
|
||
cli.add_command(info) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this file for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allows for command line commands! Try using evalml info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, got it. That's cool!
Is there a reason we should support a cmdline interface like this instead of defining evalml.info()
? What does featuretools do? What does pandas do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, we're supporting both cmdline and evalml.print_info()
; not sure if pandas has something similar but this is what featuretools does!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@angela97lin that's pretty neat!
In that case, if we're going to keep the cmdline option, we need unit test coverage for it. My suggestion: use subprocess
to run the cmd. Here's an example of how to do that using the shell script command ls -l
as an example:
In [12]: result = subprocess.Popen(['ls', '-l'], cwd='/Users/dylan.sherry/development/evalml/',
stdout=subprocess.PIPE, stderr=subprocess.PIPE)
In [13]: stdout_lines = result.stdout.readlines()
In [14]: stderr_lines = result.stderr.readlines()
In [15]: len(stderr_lines)
Out[15]: 0
In [16]: len(stdout_lines)
Out[16]: 21
Then you'll need to verify that the output contains what you expect. My advice would be to just do assert output == print_info()
, because we should have separate unit test coverage to ensure the output of print_info
is correct.
Also, I think you need to mirror featuretools and do this at the end of the file:
if __name__ == '__main__':
cli()
Here's a long explanation of why.
And if this feels like too much, an alternative is to not expose a cmdline interface and just support import evalml; evalml.print_info()
. Your call :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh lol! I just scrolled down to the test and I see you're already doing this with click
. Very cool, will read that 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm testing via test_print_cli_cmd
, which the Click documentation suggests, but lmk if you think that's not enough :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, looks good!
I think we should still add the name == main thing.
"OS-release", "machine", "processor", | ||
"byteorder", "LC_ALL", "LANG", "LOCALE"] | ||
found_keys = [k for k, _ in sys_info] | ||
assert set(info_keys).issubset(found_keys) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Wonder what else can come back from this. I'm guessing it varies by platform?
|
||
def test_print_deps_info(capsys): | ||
core_requirements = ["numpy", "pandas", "cloudpickle", "scipy", | ||
"scikit-learn", "scikit-optimize", "tqdm", "colorama"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have this get loaded from core-requirements.txt
? It would be great to have this also check requirements.txt
, and to not have to keep this list updated. Ok to file that as an separate issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to parse from core-requirements.txt
. Did you mean you want me to parse from requirements.txt
too?
@angela97lin this is awesome!! This is a great investment in our future debugging capabilities. Thanks for reviving it. I'm having trouble getting the cmdline stuff to run locally. Can you help me debug that? Here's what I get:
I tried it in bash too, not sure what's up. Any idea? I did add the Also, can we update the prints to use the logger? It'll do the same thing for now, but it'll help when we get to some of the stuff from #460, because part of the acceptance criterion for that should be that we have zero Once we get through that, I'll approve this :) |
@dsherry Thanks for the helpful comments! I'll go back and update accordingly :) RE running the command line interface: Huh, interesting. I created a new virtual env and it still worked fine for me 🤔Not sure if this would help at all but can you try making sure you've installed |
@angela97lin ah, yeah, doing a fresh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚢
Implements evalml.show_info() from featuretool's implementation