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.

Sign up for GitHub

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

Jump to bottom

Add a "dbt deps" api call (#1834) #1837

Merged
merged 7 commits into from
Oct 23, 2019
Merged

Add a "dbt deps" api call (#1834) #1837

merged 7 commits into from
Oct 23, 2019

Conversation

beckjake
Copy link
Contributor

@beckjake beckjake commented Oct 16, 2019 •
edited
Loading

Fixes #1834
Fixes #1838

  • Refactor deps:
    • split it up
    • move most of it into dbt.deps
  • Add the idea of a remote method that does not require a manifest:
    • it does still require config/args
    • set up the various reloading logic to not care about reloading these methods
  • Add dbt deps api call
    • also available as if it were from the cli, if the manifest is loaded
    • in the RPC server, deps does an shutil.rmtree on the modules_path if it exists
    • in the RPC server, deps now blocks incoming API calls, reloads the config, runs deps, and builds a new manifest
  • Add tests
  • Add new Killed and Failed task states in ps's state and poll's status
    • should we change one of those?
  • SIGHUP reloads the config before reloading the manifest
  • Added a new, slightly more reliable test suite for RPC tests and moved some flakier tests over there

@cla-bot cla-bot bot added the cla:yes label Oct 16, 2019
drewbanin
drewbanin requested changes Oct 17, 2019
View reviewed changes
Copy link
Contributor

@drewbanin drewbanin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I gave this a spin locally and it doesn't appear to me that the rpc server reparses the packages.yml file when it receives a SIGHUP. That's going to be a requirement here, as we want to be able to manage dependencies without cycling the rpc server.

It does appear that the deps task works great though -- all of that is 👍 👍 👍

We should also add a dbt clean task to let folks remove dependencies that they no longer want to include in their projects. That can certainly happen in a separate issue, or we could probably punt on it for the time being -- the rm -rf nature of dbt clean sounds... unappealing... in an environment like the rpc server.

@beckjake
Copy link
Contributor Author

Should dbt deps just rm -rf the modules folder itself?

@drewbanin
Copy link
Contributor

@beckjake yeah - i think that's the move - i don't think there's any viable use case that wouldn't benefit from cleaning out existing deps before downloading new ones. Maybe in the future dbt will check the installed deps and conditionally download new/changed ones, but today is not that day

Jacob Beck added 3 commits October 18, 2019 17:38
Add a "dbt deps" api call
abdcdef 
Refactor deps
 - split it up
 - move most of it into dbt.deps
Add the idea of a remote method that does not require a manifest
 - it does still require config/args
 - set up the various reloading logic to not care about reloading these methods
Add dbt deps api call
 - also available as if it were from the cli
Add tests
Add Kill/Fail task states + handling for them
91f0e44 
Move setting real_task into set_args on the CLI handler
Move set_args into the handle() method, and out of task bootstrap
 - now it happens in the parent/handling process
 - now the parent process can know what real task was chosen
Make the cli handler report status properly
fix a couple tests
c293fd9
@beckjake beckjake force-pushed the feature/dbt-deps-rpc branch 2 times, most recently from b816207 to be1c6f5 Compare October 21, 2019 17:09
fix the dbt deps cleanup
bf5a6b8 
dbt clean does not clean the modules path
shutil.rmtree does :)
bump test timeouts
SIGHUP also reloads the config
@drewbanin
Copy link
Contributor

I played around with this locally and some things didn't quite work as expected. This is what my testing plan looked like:

  1. start with a valid packages.yml file
  2. call the deps method on the server
  • polling for task should eventually result in a success status
  1. edit the packages.yml file to make it invalid (pick a version of a dbt package which doesn't exist) then run deps on the server
  • polling for the task should result in an error state (with a helpful error message in the logs)
  • this part didn't work quite right when I ran this branch locally -- the task looked "stuck" in a running state and the only error-related log line showed dbt runtime exception
  1. edit the packages.yml file to get it back to a valid package definition, then run deps again
  • polling for this status should eventually result in a success state

@beckjake beckjake force-pushed the feature/dbt-deps-rpc branch 2 times, most recently from 9d367a1 to fcc0110 Compare October 23, 2019 14:58
PR feedback
45bc25d 
Fix a bug where failed deps were "lost" forever
Un-support 'cli_args' + 'deps', skip those tests
Add a new test suite for RPC tests
Move flaky RPC tests into new suite where they passes like they should
@beckjake beckjake mentioned this pull request Oct 23, 2019
Merged
drewbanin
drewbanin approved these changes Oct 23, 2019
View reviewed changes
Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

This works great! Thanks for your hard work iterating on this very complex bit of server flow. The new test suite here is A+.

Ship it! :shipit:

@beckjake beckjake merged commit d43d494 into dev/louisa-may-alcott Oct 23, 2019
@beckjake beckjake deleted the feature/dbt-deps-rpc branch February 28, 2020 01:44
@dbeatty10 dbeatty10 mentioned this pull request Dec 20, 2023
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drewbanin drewbanin drewbanin approved these changes

Assignees
No one assigned
Labels
cla:yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tasks should be marked as "completed" when killed by the RPC server Support dbt deps in the rpc server
2 participants
@beckjake @drewbanin