Conversation
TomNicholas
left a comment
TomNicholas
left a comment
There was a problem hiding this comment.
Thanks @aaron-kaplan ! I'm not the main developer of this package (@maxrjones is) but this looks correct to me. A test would be great.
|
Thanks, will work on a test. At first glance, the CI failures on this PR don't seem to have anything to do with my change. Let me know if there's something I need to address. |
Hi @aaron-kaplan, thanks for this PR. The CI failures are unrelated, please ignore them for now - I'll work on fixing them later today and will give this PR a review. |
aaron-kaplan
force-pushed
the
localstore-bug
branch
from
25c650b to
5201e3d
Compare
|
After taking a look at the existing tests, I think the most straightforward way to test this behavior would be to use one of the test files from the dvc directory, but as you know dvc isn't working right now. Could you ping me when it's fixed? In order to make progress in the meantime, I dropped in one of my own test files and wrote a test using that. I'll change the filename once dvc is working again. I put the test in a commit that precedes the fix, so you can check out the first commit and see it fail, then check out the second and see it pass. If you like to keep all the tests green on every commit, I'm happy to squash them together. We could consider modifying the existing tests instead of adding a new one. I would argue that it's better practice to use LocalStore with a prefix than to include your entire filesystem hierarchy in it. |
tests/test_virtual_tiff.py
Outdated
|
|
||
| def test_local_store_with_prefix(): | ||
| data_dir = resolve_folder('tests/dvc/github').absolute() | ||
| filepath = data_dir / "chirps-v2.0.2025.01.tif" |
There was a problem hiding this comment.
| filepath = data_dir / "chirps-v2.0.2025.01.tif" | |
| filepath = data_dir / "test_reference.tif" |
This file will work. I added instructions to https://virtual-tiff.readthedocs.io/en/latest/#contributing to authenticate and pull the DVC files, or you can navigate the available files at https://dagshub.com/virtual-zarr/virtual-tiff/src/main/tests/dvc.
There was a problem hiding this comment.
FYI @aaron-kaplan the dvc setup should be fixed. I think this PR would be ready for a final check with this code suggestion
|
I failed to create an account on dagshub. I tried to authenticate with GitHub, that flow broke halfway through, and now the signup page redirects to /favicon.ico. My stack is several bugs deep at this point, so I'm not inclined to take this up with them. Just FYI. I did manage to download the file anonymously through the web interface, and confirmed that the test fails before my fix and passes after it. |
Yikes, sorry about this pain! I'll consider an alternative to DagsHub, since I'd like it to be easier for folks to contribute |
3 participants
This fixes #59 for me, and doesn't break any existing tests.
I may be getting out over my skis, and don't claim to have understood how everything fits together yet, but here's my current understanding. The word "path" in the context of the ObjectStore API means a string that identifies a file within that ObjectStore, but "path" in the context of a ChunkManifest means a full, absolute URL. In this parser, a single parameter named "path" was taken to be an ObjectStore path in some places and a ChunkManifest path in others. I've resolved that by replacing "path" with "url" or "path_in_store" as appropriate.
Tomorrow I'll try adding a test that demonstrates the bug (and the fix). In the meantime, if someone sees that I'm barking up the wrong tree, please let me know.