When metadata_url is provided, use it #63
Conversation
This is the test that we really want to have, but don't. The problem is that checking that polars will happily give you a LazyFrame that contains nothing at all.
|
I added a test that we didn't have and a note about a test that we want (which I would appreciate Becky's expertise on). FWIW, this was caused by new behavior added in #61 (specifically in f57af99) that allowed us to read from xz files. The correct procedure was followed to ensure that the code was covered, but the bug that was introduced highlighted missing specificity for these tests. It turns out that a LazyFrame is still a LazyFrame whether or not you put anything inside it. |
NOTE: do not merge this until reichlab/cladetime#63 is merged
|
@zkamvar Thanks for tackling this! You're correct about the lack of specificity (to date, the mocked S3 test setup was mostly designed to test cladetime's ability to find the correct version of a file in S3, rather than using actual file content) I'll work on incorporating better representations of data into the test setup. My best hypothesis after some digging is that presigned urls are problematic w/ polars read/scan_csv methods because polars issues a head request followed by a get, and the presigned urls can only work with a single method. It's up to you and @elray1...we can merge the patch sans tests so our scripts can work against the main branch while I'm working on this, or we can wait until the test suite is in proper shape. |
|
I vote for merge now, test later. We all understand what the bug was and it is a clear fix. The annoyance of keeping things around pointing to branches and open PRs lingering seems more severe to me than the risk of introducing an error in this PR. |
We are leaving this PR open for now to give Becky a chance to share input (generally, and also about how to test this change).