-
Notifications
You must be signed in to change notification settings - Fork 89
Removing core-requirements from installed package #1429
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
Removing core-requirements from installed package #1429
Conversation
9785ab3
to
276d008
Compare
Codecov Report
@@ Coverage Diff @@
## main #1429 +/- ##
=========================================
- Coverage 100.0% 100.0% -0.0%
=========================================
Files 218 218
Lines 14438 14433 -5
=========================================
- Hits 14431 14426 -5
Misses 7 7
Continue to review full report at Codecov.
|
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.
Looks good! Makes me wonder how useful this will be, now that we're basically printing out pip freeze
and showing a lot of packages that users may have installed that aren't relevant to evalml
(I think featuretools used this to help debug any sort of user-filed issues), but am a fan of removing core-requirements
👍
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! Its funny that the output sorts in reverse alphabetical order haha
I left one comment about test pathing but that's all I got
@angela97lin yep I hear you. I think its good to have more rather than less! We can always filter it down to the deps evalml relies on.
if x in installed_packages: | ||
packages_to_log.append((x, installed_packages[x])) | ||
for package, version in packages_to_log: | ||
for package, version in installed_packages.items(): |
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.
Ah yeah that's fine!
def test_get_core_requirements(): | ||
assert len(get_core_requirements()) == 15 | ||
def get_core_requirements(current_dir): | ||
reqs_path = os.path.join(current_dir, '../../../core-requirements.txt') |
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.
Two notes:
- This path won't work on windows, right? Interesting that the windows test still passed... Either way, safest to use
pathlib
:pathlib.Path('..', '..', '..', 'core-requirements.txt')
- I thought there was a way to have pytest automagically handle this for you somehow? Although I just spent 2min googling and couldn't find it 😅
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.
@dsherry I spent some time looking at this on the circle-ci windows vm and I don't have a satisfying explanation as to why this works...but it does lol.
Running os.path.join with these inputs returns a string with mixed windows and unix separators - so it should cause a problem:
But then open
just handles it like a champ - no idea how it works. It must be doing something to resolve the separators under the hood.
Needless to say, I'll just change this to pathlib lol.
1f3f623
to
92ac1f4
Compare
* Refactoring cli to print all dependencies. * Adding PR 1429 to release notes. * Fixing typo in PR 1429 that was causing the release_notes_updated check to fail. * Fixing import order in test_cli_utils.py * Using pathlib for get_core_requirements in test_cli_utils.py
Pull Request Description
Fixes #1328
Based on the discussion on #1293, I am printing out all of the installed packages instead of just the core requirements.
This is what the cli output looks like now:
After creating the pull request: in order to pass the release_notes_updated check you will need to update the "Future Release" section of
docs/source/release_notes.rst
to include this pull request by adding :pr:123
.