Skip to content

High verbosity mode that prints arguments, hash and store location.#1416

Merged
tomMoral merged 7 commits intojoblib:masterfrom
japhba:info-logging
Apr 17, 2023
Merged

High verbosity mode that prints arguments, hash and store location.#1416
tomMoral merged 7 commits intojoblib:masterfrom
japhba:info-logging

Conversation

@japhba
Copy link
Copy Markdown
Contributor

@japhba japhba commented Apr 9, 2023

I found this feature useful when inspecting the synchronisation of caches between a remote and local machine. In particular, it can happen that arguments are hashed differently depending on the platform.
For example, this inspection revealed that

  1. class-valued arguments were stored at a different location in memory and therefore had different hashes, as their str representation is different, and
  2. some numerical arguments that did result from a numerical did not agree because of floating point inaccuracies in their value.

In logging the arguments and their hash, I was able to track down and resolve those differences so that syncing the caches does now work.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 9, 2023

Codecov Report

Patch coverage: 96.00% and project coverage change: -0.15 ⚠️

Comparison is base (a007d79) 94.87% compared to head (5f6209b) 94.72%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1416      +/-   ##
==========================================
- Coverage   94.87%   94.72%   -0.15%     
==========================================
  Files          46       45       -1     
  Lines        6901     6918      +17     
==========================================
+ Hits         6547     6553       +6     
- Misses        354      365      +11     
Impacted Files Coverage Δ
joblib/test/test_logger.py 92.85% <ø> (ø)
joblib/test/test_memory.py 98.47% <93.33%> (-0.11%) ⬇️
joblib/logger.py 87.17% <100.00%> (+0.33%) ⬆️
joblib/memory.py 95.36% <100.00%> (+0.09%) ⬆️

... and 12 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@GaelVaroquaux
Copy link
Copy Markdown
Member

Overall the changes seem acceptable and useful. Thanks a lot.

Could you add a test that does some rough check that the added functionality works right. If we don't do this, the danger is that later changes break the functionality.

Cheers

Copy link
Copy Markdown
Contributor

@tomMoral tomMoral left a comment

Choose a reason for hiding this comment

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

A couple of comments but I agree this @GaelVaroquaux , this functionality is nice and is something that I have been thinking of for quite some time.
Thanks for the PR and for adding tests.

A couple of comments along the way on style.

info_msg = "This is a message at logging level INFO. "
debug_msg = "This is a message at logging level DEBUG. "

logging.basicConfig(level=logging.INFO)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am unsure whether this test has border effects, as the logging API relies on global handlers.
Would it be possible to create a local handler that we remove at the end of the test?

That being said, not completely sure this test is needed, maybe we could just remove it as this tests logger functionalities that are already tested by Python?

Copy link
Copy Markdown
Contributor

@tomMoral tomMoral left a comment

Choose a reason for hiding this comment

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

LGTM, thx @japhba

The failure on pypy is unrelated, so merging this one.

@tomMoral tomMoral merged commit 0454a45 into joblib:master Apr 17, 2023
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.

3 participants