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/decouple adapters from core #8906

Merged
merged 47 commits into from
Dec 22, 2023

Conversation

MichelleArk
Copy link
Contributor

@MichelleArk MichelleArk commented Oct 25, 2023

resolves #8917

Problem

Solution

Checklist

  • I have read the contributing guide and understand what's expected of me
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX
  • This PR includes type annotations for new and modified functions

MichelleArk and others added 2 commits October 25, 2023 14:56
* Move events to common

* More Type Annotations (#8536)

* Extend use of type annotations in the events module.

* Add return type of None to more __init__ definitions.

* Still more type annotations adding -> None to __init__

* Tweak per review

* Allow adapters to include python package logging in dbt logs (#8643)

* add set_package_log_level functionality

* set package handler

* set package handler

* add logging about stting up logging

* test event log handler

* add event log handler

* add event log level

* rename package and add unit tests

* revert logfile config change

* cleanup and add code comments

* add changie

* swap function for dict

* add additional unit tests

* fix unit test

* update README and protos

* fix formatting

* update precommit

---------

Co-authored-by: Peter Webb <peter.webb@dbtlabs.com>
@cla-bot cla-bot bot added the cla:yes label Oct 25, 2023
@codecov

This comment was marked as outdated.

colin-rogers-dbt and others added 5 commits October 26, 2023 15:59
* moving types_pb2.py to common/events

* split out utils into core/common/adapters

* add changie
* remove usage of dbt.config.PartialProject from dbt/adapters

* add changie

---------

Co-authored-by: Colin <colin.rogers@dbtlabs.com>
* move agate_helper into common

* add changie

---------

Co-authored-by: Colin <colin.rogers@dbtlabs.com>
@MichelleArk MichelleArk mentioned this pull request Nov 1, 2023
4 tasks
colin-rogers-dbt and others added 10 commits November 10, 2023 09:54
* moving types_pb2.py to common/events

* Refactor Base Exceptions

* update make_log_dir_if_missing to handle str

* move remaining adapters exception imports to common/adapters
---------

Co-authored-by: Michelle Ark <michelle.ark@dbtlabs.com>
* Move constraints to dbt.common

* Move constraints to contracts folder, per review

* Add a changelog entry.
* moving types_pb2.py to common/events

* Move AdapterLogger to adapter folder

* add changie
* Move the semver package to common and alter references.

* Alter leftover references to dbt.semver, this time using from syntax.

---------

Co-authored-by: Mila Page <versusfacit@users.noreply.github.com>
* moving types_pb2.py to common/events

* add config field to RelationConfig
@@ -93,11 +89,26 @@ class AdapterProtocol( # type: ignore[misc]
def __init__(self, config: AdapterRequiredConfig) -> None:
...
Copy link
Contributor

Choose a reason for hiding this comment

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

Look at macro context args

@@ -0,0 +1,275 @@
import builtins
Copy link
Contributor

Choose a reason for hiding this comment

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

Look at split of exceptions into separate files

@@ -0,0 +1,12 @@
import uuid

_INVOCATION_ID = str(uuid.uuid4())
Copy link
Contributor

Choose a reason for hiding this comment

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

new invocation_id global

@colin-rogers-dbt colin-rogers-dbt marked this pull request as ready for review December 19, 2023 17:49
@colin-rogers-dbt colin-rogers-dbt requested review from heysweet and gshank and removed request for a team December 19, 2023 17:49
from dbt.common.clients.jinja import MacroProtocol


class MacroResolverProtocol(Protocol):
Copy link
Contributor

Choose a reason for hiding this comment

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

replaces referencing the manifest

_EVENT_MANAGER: IEventManager = EventManager()


def get_event_manager() -> IEventManager:
Copy link
Contributor

Choose a reason for hiding this comment

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

this functionality was moved from functions.py to make it clearer how the event manager is setup and managed

from dbt.common.events.logger import LineFormat

# make sure event manager starts with a logger
get_event_manager().add_logger(
Copy link
Contributor

Choose a reason for hiding this comment

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

this behavior was in factory.py, we have moved it to init as this is a more idiomatic place to locate behavior that we expect to happen on module import

* moving types_pb2.py to common/events

* Update parser to support conversion metrics (#9173)

* added ConversionTypeParams classes

* updated parser for ConversionTypeParams

* added step to populate input_measure for conversion metrics

* version bump on DSI

* comment back manifest generating line

* updated v12 schemas

* added tests

* added changelog

* Remove `--dry-run` flag from `dbt deps` (#9169)

* Rm --dry-run flag for dbt deps

* Add changelog entry

* Update test

* PR feedback

* adding clean_up methods to basic and unique_id tests (#9195)

* init attempt of adding clean_up methods to basic and unique_id tests

* swapping cleanup method drop of test_schema to unique_schema to test breakage on docs_generate test

* moving the clean_up method down into class BaseDocsGenerate

* remove drop relation for unique_schema

* manually define alternate_schema for clean_up as not being seen as part of project_config

* add changelog

* remove unneeded changelog

* uncomment line that generates new manifest and delete manifest our changes created

* make sure the manifest test is deleted and readd older version of manifest.json to appease test

* manually revert file to previous commit

* Revert "manually revert file to previous commit"

This reverts commit a755419.

---------

Co-authored-by: William Deng <33618746+WilliamDee@users.noreply.github.com>
Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com>
Co-authored-by: Matthew McKnight <91097623+McKnight-42@users.noreply.github.com>
Copy link
Contributor

@ChenyuLInx ChenyuLInx left a comment

Choose a reason for hiding this comment

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

Thanks for getting the change up!! This looks mostly good to me!
Have we tried this branch with other 1-rst party adapters to get some rough understanding of how much work one adapter need to use this change?

@@ -0,0 +1,7 @@
kind: Breaking Changes
body: Rm --dry-run flag from 'dbt deps --add-package', in favor of just 'dbt deps
Copy link
Contributor

Choose a reason for hiding this comment

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

This change seems not so relevant to what you are doing in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

yea I'm not sure why this shows as a diff as the PR for this was merged last week?

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -0,0 +1,6 @@
kind: Features
body: Remove legacy logger
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't use this on cloud anymore, do we need to double check?

Copy link
Contributor

Choose a reason for hiding this comment

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

@MichelleArk checked and they have all moved to event logging

@colin-rogers-dbt
Copy link
Contributor

Thanks for getting the change up!! This looks mostly good to me! Have we tried this branch with other 1-rst party adapters to get some rough understanding of how much work one adapter need to use this change?

@ChenyuLInx yup I have a working PR up for bigquery: dbt-labs/dbt-bigquery#1026

* moving types_pb2.py to common/events

* Update parser to support conversion metrics (#9173)

* added ConversionTypeParams classes

* updated parser for ConversionTypeParams

* added step to populate input_measure for conversion metrics

* version bump on DSI

* comment back manifest generating line

* updated v12 schemas

* added tests

* added changelog

* Remove `--dry-run` flag from `dbt deps` (#9169)

* Rm --dry-run flag for dbt deps

* Add changelog entry

* Update test

* PR feedback

* adding clean_up methods to basic and unique_id tests (#9195)

* init attempt of adding clean_up methods to basic and unique_id tests

* swapping cleanup method drop of test_schema to unique_schema to test breakage on docs_generate test

* moving the clean_up method down into class BaseDocsGenerate

* remove drop relation for unique_schema

* manually define alternate_schema for clean_up as not being seen as part of project_config

* add changelog

* remove unneeded changelog

* uncomment line that generates new manifest and delete manifest our changes created

* make sure the manifest test is deleted and readd older version of manifest.json to appease test

* manually revert file to previous commit

* Revert "manually revert file to previous commit"

This reverts commit a755419.

---------

Co-authored-by: William Deng <33618746+WilliamDee@users.noreply.github.com>
Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com>
Co-authored-by: Matthew McKnight <91097623+McKnight-42@users.noreply.github.com>
colin-rogers-dbt and others added 2 commits December 22, 2023 09:03
* Update parser to support conversion metrics (#9173)

* added ConversionTypeParams classes

* updated parser for ConversionTypeParams

* added step to populate input_measure for conversion metrics

* version bump on DSI

* comment back manifest generating line

* updated v12 schemas

* added tests

* added changelog

* Remove `--dry-run` flag from `dbt deps` (#9169)

* Rm --dry-run flag for dbt deps

* Add changelog entry

* Update test

* PR feedback

* adding clean_up methods to basic and unique_id tests (#9195)

* init attempt of adding clean_up methods to basic and unique_id tests

* swapping cleanup method drop of test_schema to unique_schema to test breakage on docs_generate test

* moving the clean_up method down into class BaseDocsGenerate

* remove drop relation for unique_schema

* manually define alternate_schema for clean_up as not being seen as part of project_config

* add changelog

* remove unneeded changelog

* uncomment line that generates new manifest and delete manifest our changes created

* make sure the manifest test is deleted and readd older version of manifest.json to appease test

* manually revert file to previous commit

* Revert "manually revert file to previous commit"

This reverts commit a755419.

---------

Co-authored-by: William Deng <33618746+WilliamDee@users.noreply.github.com>
Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com>
Co-authored-by: Matthew McKnight <91097623+McKnight-42@users.noreply.github.com>
@colin-rogers-dbt colin-rogers-dbt merged commit 7763212 into main Dec 22, 2023
50 of 51 checks passed
runleonarun pushed a commit to dbt-labs/docs.getdbt.com that referenced this pull request Jan 5, 2024
## What are you changing in this pull request and why?
<!---
Describe your changes and why you're making them. If related to an open 
issue or a pull request on dbt Core, then link to them here! 

To learn more about the writing conventions used in the dbt Labs docs,
see the [Content style
guide](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/content-style-guide.md).
-->
This PR updates various links after a [PR in dbt
Core](dbt-labs/dbt-core#8906) moved some files.
Additionally, it updates some of the links to point to the correct line
of code.

## Checklist
<!--
Uncomment when publishing docs for a prerelease version of dbt:
- [ ] Add versioning components, as described in [Versioning
Docs](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/single-sourcing-content.md#versioning-entire-pages)
- [ ] Add a note to the prerelease version [Migration
Guide](https://github.com/dbt-labs/docs.getdbt.com/tree/current/website/docs/docs/dbt-versions/core-upgrade)
-->
- [x] Review the [Content style
guide](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/content-style-guide.md)
so my content adheres to these guidelines.
- [ ] For [docs
versioning](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/single-sourcing-content.md#about-versioning),
review how to [version a whole
page](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/single-sourcing-content.md#adding-a-new-version)
and [version a block of
content](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/single-sourcing-content.md#versioning-blocks-of-content).
- [ ] Add a checklist item for anything that needs to happen before this
PR is merged, such as "needs technical review" or "change base branch."

Adding or removing pages (delete if not applicable):
- [ ] Add/remove page in `website/sidebars.js`
- [ ] Provide a unique filename for new pages
- [ ] Add an entry for deleted pages in `website/static/_redirects`
- [ ] Run link testing locally with `npm run build` to update the links
that point to deleted pages

Co-authored-by: mirnawong1 <89008547+mirnawong1@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-3270] [Epic] Decouple dbt/adapters from core
5 participants