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

Fsspec remfile with cache #43

Merged
merged 25 commits into from
Apr 24, 2024
Merged

Fsspec remfile with cache #43

merged 25 commits into from
Apr 24, 2024

Conversation

sinha-r
Copy link
Collaborator

@sinha-r sinha-r commented Apr 18, 2024

Implementation for fsspec with cache and remfile with cache.

  • A tmpdir was added for both cases to facilitate cleanup.

Fix #45

Implementation for remfile and fsspec, both with cache
Implemented changes for fsspec and remfile, both with cache
Implemented changes for fsspec and remfile, both with cache
Implemented changes for remfile and fsspec, both with cache
Implemented changes for both fsspec and remfile, both with cache
@sinha-r sinha-r added the category: enhancement improvements of code or code behavior label Apr 18, 2024
Copy link
Contributor

@oruebel oruebel left a comment

Choose a reason for hiding this comment

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

It looks like there are a few issues with indentation, but otherwise this looks good to me.

@CodyCBakerPhD
Copy link
Collaborator

Looks good to me minus the syntax issues - @sinha-r can you ensure that you can successfully run the new benchmarks before we merge?

@CodyCBakerPhD
Copy link
Collaborator

One more thing I just now realized - apparently doing del tmpdir doesn't remove the directory on my system - can you add a teardown method (this will be called automatically at end of each check, very similar to unittest.TestCase.TearDown) to each benchmark using a temporary cache?

@oruebel
Copy link
Contributor

oruebel commented Apr 24, 2024

One more thing I just now realized - apparently doing del tmpdir doesn't remove the directory on my system - can you add a teardown method

Strange, the main point of TemporaryDirectory is that it is supposed to clean up automatically, at the very latest once the Python garbage collector cleans up the object. Maybe an error in the tests caused the clean-up to fail. I've set the ignore_cleanup_errors=True parameter to make sure it tries to also clean up when it encounters issues and I've added teardown functions to the corresponding tests to also explicitly call the cleanup method of the TemporaryDirectory, rather than relying on the garbage collector. Hopefully that fixes that issue you were seeing.

@oruebel
Copy link
Contributor

oruebel commented Apr 24, 2024

I've fixed the errors I found and am running the benchmarks now to make sure they work. Otherwise this seems good to merge now.

Copy link
Contributor

@oruebel oruebel left a comment

Choose a reason for hiding this comment

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

It looks like all the tests ran on my machine at work. Good to merge.

@oruebel oruebel merged commit 3d65167 into main Apr 24, 2024
2 checks passed
@oruebel oruebel deleted the fsspec_remfile_with_cache branch April 24, 2024 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: enhancement improvements of code or code behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement RemFIle with DiskCache test
3 participants