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

Feature: contexts cleanup #2085

Merged
merged 3 commits into from
Feb 7, 2020
Merged

Conversation

beckjake
Copy link
Contributor

@beckjake beckjake commented Jan 31, 2020

Fix #1503
Fix #1981 (you can still access it via ref, but you really shouldn't)
Fix #1255

Rewrite/reorganize contexts

  • now they are objects that have a to_dict() (but not dataclasses, and not JsonSchemaMixins)
  • special decorators on properties/methods to indicate membership in the context (vs helper methods). It is some more metaclass nonsense. We could use them to generate a list of what's in the context (and maybe even auto-generate docs about contexts?).
  • reorganized the dbt.context package to remove/avoid most import cycles

High-level context behavior changes:

  • 'dbt_version' is always available
  • 'target' is available as long as the profile has been parsed
  • 'project_name' is available in: query headers, hooks, models, and macro execution
  • 'builtins' now includes everything in the context except macros, not just the base context. I'm not sure if that's exactly perfect, but it feels pretty right.
  • I had to make zero actual integration test changes (a great sign!) but I probably broke some of our untested/under-tested context members :(

Profiles/projects now render with more complete contexts

  • reading a profile or project file requires a ConfigRenderer, which accepts a context dict as its argument
  • projects get an extra-fiddly special load for use during initial parsing
    • it returns the profile name and a function that, given a renderer, can return the rest of the Project
    • idea is: use the profile name to load the Profile, use that to build a ConfigRenderer that has a TargetContext, render the project with that
  • profiles.yml is rendered with the 'base' context
  • dbt_project.yml/schema.yml/packages.yml are rendered with the 'target' context: 'base' context + 'target'
  • docs are rendered with the 'docs' context: 'target' context + the 'doc' function
  • query headers are rendered with the query header context: 'target' context + macros + 'project_name' field
  • executed macros/models should have the same context as previously (query headers + adapter/model/etc related functions)

Moved actual ref/source searching into the manifest
Moved the rest of the parse utils into parser/manifest.py
Made the ref/source resolvers defined in the provider context a bit more sane/well-defined
Picked consistent-but-not-great names for all the various context generation functions
Moved write_node into ParsedNode
I had to thread the ConfigRenderer all through the deps code because way down in there, it wants to read projects for metadata reasons. I'm not sure what the best way to fix that is and I don't want to do it in this PR, but deps definitely shouldn't do that.

I'd like to write some net-new tests, but here is an initial pass that shouldn't break much of anything (though adding everything to the context may in fact do so).

@cla-bot cla-bot bot added the cla:yes label Jan 31, 2020
@beckjake beckjake changed the title Feature: contexts redux Feature: contexts cleanup Feb 3, 2020
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.

I just did a first pass here. I'd like to spend some more time functional testing this, but did want to get some initial comments on your radar!

Overall, wow, this is a heroic looking PR. Looking forward to getting this merged!

# project_profile_name is ignored, we just need it to appease mypy
# (Profile.from_args uses it)
# profile_name from the project
partial = Project.partial_load(os.getcwd())
Copy link
Contributor

Choose a reason for hiding this comment

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

👀 do we want os.getcwd() here? Does running dbt from a subdir still work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. the relative directory handling stuff actually uses chdir to go up the file tree before this is ever called, so the current working directory is in fact fine here.

)

# get a new renderer using our target information and render the
Copy link
Contributor

Choose a reason for hiding this comment

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

this feels totally appropriate and sensible to me!

core/dbt/context/configured.py Outdated Show resolved Hide resolved
core/dbt/context/configured.py Outdated Show resolved Hide resolved
core/dbt/contracts/graph/manifest.py Outdated Show resolved Hide resolved
Jacob Beck added 2 commits February 6, 2020 08:40
Rewrite/reorganize contexts
 - now they are objects that have a to_dict()
 - special decorators on properties/methods to indicate membership
 - reorganize contexts/ to remove many many import cycles

Context behavior changes:
 - 'dbt_version' is alwways available
 - 'target' is available as long as the profile has been parsed
 - 'project_name' is available in: query headers, hooks, models, and macro execution

Profiles/projects now render at load time
 - reading a profile or project file requires a ConfigRenderer
 - projects get an extra-fiddly special load for use during initial parsing
    - it returns the profile name and a function that, given a renderer, can return the rest of the Project
    - idea is: use the profile name to load the Profile, use that to build a ConfigRenderer that has a TargetContext, render the project with that
 - profiles.yml is rendered with the 'base' context
 - dbt_project.yml/schema.yml/packages.yml are rendered with the 'target' context: 'base' context + 'target'
 - docs are rendered with the docs context: 'target' context + the 'doc' function
 - query headers are rendered with the query header context: 'target' context + macros + 'project_name'
 - executed macros/models should have the same context as previously (query headers + adapter/model/etc related functions)

Moved actual ref/source searching into the manifest
Moved the rest of the parse utils into parser/manifest.py
Made the ref/source resolvers defined in the provider context a bit more sane/well-defined
Picked consistent-but-not-great names for all the various context generation functions
Moved write_node into ParsedNode
Make slight tweaks to project initialization to support tests
@beckjake
Copy link
Contributor Author

beckjake commented Feb 6, 2020

Rebased to handle a merge conflict

Fix comments
Implement a TODO around duplicate macro names
 - added unit tests for it
Implement a TODO around ref resolution
@beckjake
Copy link
Contributor Author

beckjake commented Feb 7, 2020

Per slack discussion, merging this.

@beckjake beckjake merged commit 9df123a into dev/barbara-gittings Feb 7, 2020
@beckjake beckjake deleted the feature/contexts-redux branch February 28, 2020 01:42
mmeasic added a commit to mmeasic/docs.getdbt.com that referenced this pull request Jan 17, 2021
The `project_name` is not available in the `dbt_project.yml` context. 
'project_name' is available in: query headers, hooks, models, and macro execution

See: dbt-labs/dbt-core#2085
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants