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

Implement fine-grained file cache control options #314

Merged
merged 18 commits into from Feb 8, 2023

Conversation

pjbull
Copy link
Member

@pjbull pjbull commented Jan 23, 2023

Implements fine-grained controls for file cache clearing.

Best overview is the update to caching.ipynb, which you can see in the docs preview:
https://deploy-preview-314--gallant-agnesi-5f7bb3.netlify.app/caching/#clearing-the-file-cache

Probably worth it to start with reading the docs since we want those to be a clear explanation of the change.

Bonus:

  • Our file closing was not idempotent (if .close() was called twice, the file would upload twice). This usually resulted in a failure to overwrite because modified times were the same. Fixed to not upload if close has already been called.
  • Hide input/output numbers in docs generated by jupyter notebooks

Closes #10

@github-actions
Copy link
Contributor

github-actions bot commented Jan 23, 2023

@codecov-commenter
Copy link

codecov-commenter commented Jan 23, 2023

Codecov Report

Merging #314 (2e0b336) into master (bb9d0a3) will decrease coverage by 0.2%.
The diff coverage is 97.5%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #314     +/-   ##
========================================
- Coverage    94.2%   94.1%   -0.2%     
========================================
  Files          21      22      +1     
  Lines        1396    1461     +65     
========================================
+ Hits         1316    1375     +59     
- Misses         80      86      +6     
Impacted Files Coverage Δ
cloudpathlib/client.py 89.1% <92.5%> (+1.2%) ⬆️
cloudpathlib/azure/azblobclient.py 93.9% <100.0%> (+<0.1%) ⬆️
cloudpathlib/cloudpath.py 92.8% <100.0%> (+0.2%) ⬆️
cloudpathlib/enums.py 100.0% <100.0%> (ø)
cloudpathlib/exceptions.py 100.0% <100.0%> (ø)
cloudpathlib/gs/gsclient.py 92.8% <100.0%> (-1.6%) ⬇️
cloudpathlib/local/localclient.py 96.6% <100.0%> (+<0.1%) ⬆️
cloudpathlib/s3/s3client.py 92.4% <100.0%> (-1.5%) ⬇️

@pjbull pjbull marked this pull request as ready for review January 25, 2023 07:32
@pjbull pjbull requested a review from jayqi January 25, 2023 07:32
@pjbull pjbull changed the title WIP Implement fine-grained file cache control options Implement fine-grained file cache control options Jan 25, 2023
Copy link
Member

@jayqi jayqi left a comment

Choose a reason for hiding this comment

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

This looks awesome! Will be a great new feature.

I saw a bunch of typos in the caching notebook—wondering if we should nbautoexport it for PR review purposes.

cloudpathlib/enums.py Outdated Show resolved Hide resolved
cloudpathlib/enums.py Show resolved Hide resolved
cloudpathlib/azure/azblobclient.py Show resolved Hide resolved
@pjbull pjbull force-pushed the 10-implement-new-cache-modes branch from 3c9fe99 to bac8e73 Compare February 4, 2023 01:54
@pjbull
Copy link
Member Author

pjbull commented Feb 4, 2023

@jayqi Should be ready for re-review, sorry had to rebase. New commits are from 6180bdb onwards. You can probably ignore c121e65, which is just running black on the notebooks.

docs/docs/script/caching.py Outdated Show resolved Hide resolved
docs/docs/script/caching.py Outdated Show resolved Hide resolved
docs/docs/script/caching.py Outdated Show resolved Hide resolved
docs/docs/script/caching.py Outdated Show resolved Hide resolved
docs/docs/script/caching.py Outdated Show resolved Hide resolved
@pjbull
Copy link
Member Author

pjbull commented Feb 8, 2023

Thanks @jayqi! I incorporated all your suggestions

Copy link
Member

@jayqi jayqi left a comment

Choose a reason for hiding this comment

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

One remaining typo. LGTM otherwise!

docs/docs/script/caching.py Outdated Show resolved Hide resolved
pjbull and others added 2 commits February 8, 2023 11:00
Co-authored-by: Jay Qi <2721979+jayqi@users.noreply.github.com>
@pjbull pjbull merged commit 088fe49 into master Feb 8, 2023
@pjbull pjbull deleted the 10-implement-new-cache-modes branch February 8, 2023 20:55
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.

Add ability to turn off local cache
3 participants