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

Make all references to the logger be static #503

merged 10 commits into from Mar 17, 2020


Copy link

@dsherry dsherry commented Mar 16, 2020

Fixes #343

@dsherry dsherry self-assigned this Mar 16, 2020
@dsherry dsherry changed the title [WIP] Make all references to the logger be static Make all references to the logger be static Mar 16, 2020
@dsherry dsherry requested review from kmax12, angela97lin and jeremyliweishih and removed request for kmax12 Mar 16, 2020
Copy link

codecov bot commented Mar 16, 2020

Codecov Report

Merging #503 into master will increase coverage by 0.01%.
The diff coverage is 92.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #503      +/-   ##
+ Coverage   98.42%   98.43%   +0.01%     
  Files         104      105       +1     
  Lines        3427     3450      +23     
+ Hits         3373     3396      +23     
  Misses         54       54              
Impacted Files Coverage Δ
evalml/pipelines/components/ 93.10% <66.66%> (ø)
evalml/automl/ 93.30% <84.21%> (+0.02%) ⬆️
evalml/pipelines/ 97.61% <100.00%> (ø)
evalml/tests/utils_tests/ 100.00% <100.00%> (ø)
evalml/utils/ 100.00% <100.00%> (ø)
evalml/utils/ 95.23% <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 44142d6...ef87e0a. Read the comment docs.

Copy link

@angela97lin angela97lin left a comment

Nice! Seems like codecov is failing though because we're not checking for calls to it in AutoBase. What do you think about adding a test or two to check for captured output? (Quite frankly, seems a little unncessary but /shrug)

def __init__(self, verbose=True):
self.verbose = True
Copy link

@angela97lin angela97lin Mar 17, 2020

Choose a reason for hiding this comment

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

Nice catch!

docs/source/changelog.rst Outdated Show resolved Hide resolved
kmax12 approved these changes Mar 17, 2020
Copy link

@kmax12 kmax12 left a comment

Nice. LGTM

Copy link

@jeremyliweishih jeremyliweishih left a comment

Looks good to me. Should we think about having a singleton logger in the future?

Copy link
Collaborator Author

dsherry commented Mar 17, 2020

Thanks all for review.

@angela97lin good point about codecov! I see in the diff (thx for your help finding that lol) that the codecov patch check is complaining because I added 1 new line in auto_base... 🤦‍♂️haha. I agree it would be nice to have logger-level coverage of auto_base, but I don't think its worth our time to add that until we do some more logger overhauling. That said, I don't want us to get in a habit of ignoring codecov... maybe there's a way to relax the limits a bit?

@jeremyliweishih yeah we definitely should move towards a singleton logger! There's a lot of improvements we can make. I wanted to start small.

Copy link

angela97lin commented Mar 17, 2020

@dsherry Yup, agreed, don't think it's worth right now.

Hmmm, what are you thinking about with regards to relaxing the limits? We could decrease the % necessary to pass the test... But I think alternatively, it could be useful to keep the limit as is--that forces us to check why codecov has decreased, and if it's for a silly reason, at least we can ignore it with justification :D

Copy link
Collaborator Author

dsherry commented Mar 17, 2020

@angela97lin good point. We shouldn't change the codecov limits; we should just provide good justification when we choose to ignore codecov.

@dsherry dsherry merged commit c088b70 into master Mar 17, 2020
1 of 2 checks passed
@dsherry dsherry deleted the ds_343_logger_static branch Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

Have all references to the logger be static
4 participants