-
Notifications
You must be signed in to change notification settings - Fork 120
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
Local file cache #696
Local file cache #696
Conversation
Codecov Report
@@ Coverage Diff @@
## main #696 +/- ##
=====================================
Coverage 94.8% 94.9%
=====================================
Files 58 58
Lines 5856 5900 +44
=====================================
+ Hits 5557 5600 +43
- Misses 299 300 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @Rlamboll!
I'm worried about two issues:
- By accepting only csv files for the local copy, you loose all meta indicators, which can be very relevant
- The query is not actually stored, so if you do (as you do in the test
and then
df = lazy_read_iiasa("some/file.csv", TEST_API, model="model_a")
you will get a different result for the second call depending on whether any data changed in the database in the meantime.df = lazy_read_iiasa("some/file.csv", TEST_API, model="model_b")
c13f7c3
to
67a478e
Compare
I've enabled it to read xls(x) files, but I don't think it's possible to do proper checks that the filters are applied in only a more restrictive sense without spending a significant period of time querying the database, which is what we're trying to avoid in the first place. I've expanded the documentation to make it clear that it's on the user to look out for this. This is consistent with how the code is currently used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% convinced about this new feature because there might be confusion when a user queries for different filters than what is stored in the local cache. I'll approve, merge, and then create an issue for future improvements.
Done to resolve #681
Please confirm that this PR has done the following:
Adding to RELEASE_NOTES.md (remove section after adding to RELEASE_NOTES.md)
Please add a single line in the release notes similar to the following:
Description of PR
Please describe the changes introduced by this PR.