-
Notifications
You must be signed in to change notification settings - Fork 3
Support BSON and JLD2 file formats #19
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
Conversation
Co-authored-by: dahong67 <9384655+dahong67@users.noreply.github.com>
Co-authored-by: dahong67 <9384655+dahong67@users.noreply.github.com>
Co-authored-by: dahong67 <9384655+dahong67@users.noreply.github.com>
Co-authored-by: dahong67 <9384655+dahong67@users.noreply.github.com>
|
@copilot let's drop FileIO since some filetypes (e.g., PNG) don't actually make much sense for the general purpose caching we are doing (where we want to be able to store "arbitrary" Julia objects). And saving outputs to application-specific formats is probably best left to actually just using FileIO (or the relevant file saving/loading packages) directly. Instead, let's stick with just BSON and JLD2. Filenames with other extensions should error. Also, let's not include MAT since it doesn't seem to be able to round-trip data (e.g., in my testing it seemed to turn |
…sions Co-authored-by: dahong67 <9384655+dahong67@users.noreply.github.com>
Done! Removed FileIO dependency and now using BSON and JLD2 directly. Files with unsupported extensions (anything other than Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19 +/- ##
===========================================
- Coverage 100.00% 96.05% -3.95%
===========================================
Files 1 1
Lines 43 76 +33
===========================================
+ Hits 43 73 +30
- Misses 0 3 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Okay, I'll do some work on this now
|
@copilot please add tests to get coverage back to 100% |
…dling, and cachemeta Co-authored-by: dahong67 <9384655+dahong67@users.noreply.github.com>
Done! Added comprehensive tests to cover all code paths:
All 72 tests now pass. Also fixed a bug in the macro form where JLD2 keys needed proper handling (JLD2 converts Symbol keys to String keys when saving). (4053b6f) Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot please redo this on top of the current master |
Done! Rebased all JLD2 support changes on top of current master (904f01a). The changes are now compatible with the refactored API from PR #20. (add4c53) Note: This is a rebase, so the branch history has been rewritten. The old commits have been replaced with new ones based on master. Main changes:
|
|
Looks like this rebase may have been too complicated for copilot. Let’s try again with a fresh PR. |
Rebased on master ✅
Successfully rebased JLD2 support on top of the current master branch (commit 904f01a) which includes the major API refactoring from PR #20.
Changes Made
.bsonor.jld2)@cachewith.jld2)Compatibility with New API
The implementation works with the refactored API from master:
cache()function now includes metadata by default (wascachemeta())@cachemacro works with both formatsbson_modparameter supported for BSON module loadingTesting
All 70 tests pass including:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.