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

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

Merged
merged 7 commits into from
Apr 17, 2023

Conversation

japhba
Copy link
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

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
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
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.

joblib/memory.py Outdated Show resolved Hide resolved
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
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?

joblib/test/test_logger.py Outdated Show resolved Hide resolved
Copy link
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.

None yet

3 participants