Skip to content
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

Create Hourly Spellbook subproject #6401

Closed
wants to merge 13 commits into from

Conversation

0xRobin
Copy link
Collaborator

@0xRobin 0xRobin commented Jul 22, 2024

  1. Split out hourly models into its' own new 'hourly_spellbook' dbt subproject
├── _project
│   ├── balancer
│   ├── cow_protocol
│   ├── ens
│   ├── lido
│   └── safe
└── _sector
    ├── abi
    ├── account_abstraction
    ├── blobs
    ├── cex
    ├── lending
    ├── perpetual
    ├── prices
    ├── rollup_economics
    └── staking

  1. This PR also moves the following lineages to the tokens subproject:
- prices.tokens
- transfers
- balances

@0xRobin 0xRobin changed the title split out hourly models Create Hourly Spellbook subproject Jul 23, 2024
@0xRobin 0xRobin added ready-for-review this PR development is complete, please review do not merge dune team created by dune team labels Jul 23, 2024
@0xRobin 0xRobin marked this pull request as ready for review July 23, 2024 10:44
@0xRobin 0xRobin requested a review from jeff-dude July 23, 2024 12:06
@jeff-dude jeff-dude self-assigned this Jul 23, 2024
@jeff-dude jeff-dude added in review Assignee is currently reviewing the PR and removed ready-for-review this PR development is complete, please review do not merge labels Jul 23, 2024
Copy link
Member

@jeff-dude jeff-dude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in addition to the minor comments below:

  • any reason we don't move project specific macros to their new subproject, i.e. all the balancer macros to hourly_spellbook/macros/? i think we're gonna have some follow up cleaning to do in terms of getting all macros in the correct spot, across various projects
  • can we use this PR to continue moving more around and finalize the projects? my thought was to reuse the existing dbt cloud project and cluster for hourly_spellbook, then all others move to their final spots too, which would require no extra setup since already done. that would mean pushing all remaining spells to daily_spellbook project within this PR too. otherwise, we'll have to set up new dbt cloud project, new cluster, to support hourly_spellbook while we continue to run the other jobs on legacy

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason transfers has its own source file here rather than add to _sources?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no particular reason, I was trying some new formats out to see how we can clean up the shared sources in the future.
I think I do prefer this setup with some more structure and split into more different files.
From a dbt standpoint this doesn't matter at all so it's just a repository organisation question.

@jeff-dude jeff-dude added the question Further information is requested label Jul 23, 2024
@0xRobin
Copy link
Collaborator Author

0xRobin commented Jul 24, 2024

  • any reason we don't move project specific macros to their new subproject, i.e. all the balancer macros to hourly_spellbook/macros/? i think we're gonna have some follow up cleaning to do in terms of getting all macros in the correct spot, across various projects

good point, will clean them up while we're at it. 👍

  • can we use this PR to continue moving more around and finalize the projects? my thought was to reuse the existing dbt cloud project and cluster for hourly_spellbook, then all others move to their final spots too, which would require no extra setup since already done. that would mean pushing all remaining spells to daily_spellbook project within this PR too. otherwise, we'll have to set up new dbt cloud project, new cluster, to support hourly_spellbook while we continue to run the other jobs on legacy

Initially I wanted to limit the PR to just splitting out the hourly models (as it is already huge).
But happy to take this further in one go if we're fine with the CI not running. 👌

@0xRobin
Copy link
Collaborator Author

0xRobin commented Jul 24, 2024

yeah github is not liking these big PRs, will revert and split back into 2.

@0xRobin
Copy link
Collaborator Author

0xRobin commented Jul 24, 2024

@jeff-dude my advise would be to merge this, setup a new dbt cloud job for hourly spellbook but re-use the current legacy cluster.
After that we can open the follow-up PR against main and move everything to it's final location.

@0xRobin
Copy link
Collaborator Author

0xRobin commented Jul 24, 2024

I'm closing this and reopening the PR here (#6421) from a branch within the spellbook repo (instead of my fork).
Ci is behaving weird and I think it's due to the branch being on the fork.

@0xRobin 0xRobin closed this Jul 24, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jul 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dune team created by dune team in review Assignee is currently reviewing the PR question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants