-
Notifications
You must be signed in to change notification settings - Fork 4
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
Update 2025 input data #66
Update 2025 input data #66
Conversation
# Adds arrow support to speed up ingest process | ||
noctua_options(unload = TRUE) | ||
|
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.
We hadn't yet switched to using the unload = TRUE
method for noctua
, meaning array columns were still being pulled in as comma-separated strings. Most of the updates in this file are to implement the same array handling used in the res model.
"RemoteUsername": "DyfanJones", | ||
"RemoteRepo": "noctua", | ||
"RemoteRef": "master", | ||
"RemoteSha": "23a4cfbf537407c7a1547fc13ba771ba2eb098e0", |
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.
noctua
didn't actually get bumped (in #62) to the version with the unload
fix, so this PR does that as well as bumping/adding a few other dependencies.
14ce8e2
to
7e48669
Compare
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.
This will be our 2024 data setup until we get 2024 sales and data sorted out. I'll dvc push once this is merged.
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 picking this up! One small question but it's not serious.
deps: | ||
- path: pipeline/00-ingest.R | ||
hash: md5 | ||
md5: 29292ee2bef109914c423c9259aa8879 | ||
size: 22847 |
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.
[Question, non-blocking] I haven't seen us use this pattern before, what's the goal of making the script a dependency?
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.
It's used in both model repos now, with the thinking being that the pipeline should be restarted if the script changes. I'm not super tied to this setup though if you don't like it. I agree it's a bit weird.
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 think that makes sense, thanks!
A few updates here to reflect the changes from ccao-data/model-res-avm#283, as well as catch the condo model up on the
unload = TRUE
option ofnoctua
.