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

Remove the duplicate get_context_modules #1996

Merged
merged 3 commits into from
Dec 10, 2019

Conversation

Fokko
Copy link
Contributor

@Fokko Fokko commented Dec 10, 2019

While looking at the code, and figuring out how to inject custom modules, I've stumbled upon some duplicate code. Besides that, I also took the liberty of adding annotations.

@cla-bot cla-bot bot added the cla:yes label Dec 10, 2019
@michael-erasmus
Copy link

michael-erasmus commented Dec 10, 2019

Wow, that's crazy @Fokko, I was just preparing a PR to do the exact same thing as you (add the ability to inject custom modules), also came across this duplicated code and then I spotted your PR. Do you have a follow-up PR for injecting modules? If not, I'll merge your code first to do my PR :)

@Fokko
Copy link
Contributor Author

Fokko commented Dec 10, 2019

Good to see that I'm not the only one :-) I'd like to do some counts, and send them to an external system.

I'm currently exploring two directions:

  • Inject the function in some messy way:
def hello():
    return 'world'

dbt.context.base.CONTEXT_MODULES['custom'] = {
    'my_custom_function': hello
}

dbt.main()
  • Do it using a Spark UDF. But I don't like this, because then we also have to distribute credentials to Spark.

Copy link
Contributor

@beckjake beckjake 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 the PR @Fokko - Funny story, I actually have a WIP branch that also fixes this, but it's not ready to go for unrelated reasons.

I'm happy to merge this standalone, some very minor feedback before I kick off the tests and we get this merged :)

'pytz': get_pytz_module_context(),
'datetime': get_datetime_module_context(),
}
CONTEXT_MODULES: Dict[str, Dict[str, Any]] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you keep this as a global function that returns a dict, instead of a global dict? I have a good reason and a bad/lazy reason:

  • I don't want us to share the same dict into every jinja environment. I think jinja does a shallow copy of the context and I want to build a fresh one every time we generate a context, since dicts are mutable.
  • I want to minimize merge conflicts with my other branch that makes the same change :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I've reverted the change.

@Fokko
Copy link
Contributor Author

Fokko commented Dec 10, 2019

I've stopped pursuing implementing a custom Python function, and went with https:/fishtown-analytics/dbt-event-logging/ instead. This does everything I need for now.

Copy link
Contributor

@beckjake beckjake left a comment

Choose a reason for hiding this comment

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

I've kicked off the test suite, I think this is good to go. Thanks for your contribution to dbt!

@beckjake
Copy link
Contributor

@Fokko sorry about this, but mypy is failing, I think before this function wasn't annotated so it wasn't inspected. Can you add a # type: ignore to the line that accesses pytz.__all__? That attribute does exist...

@Fokko
Copy link
Contributor Author

Fokko commented Dec 10, 2019

@beckjake Sure, I've added the exception.

@beckjake beckjake merged commit 40839c7 into dbt-labs:dev/0.15.1 Dec 10, 2019
@Fokko Fokko deleted the remove-duplicates branch January 5, 2020 17:34
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.

3 participants