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 UnsetProfileConfig #6504

Merged
merged 2 commits into from
Jan 7, 2023

Conversation

MichelleArk
Copy link
Contributor

@MichelleArk MichelleArk commented Jan 3, 2023

resolves #5539

Description

Checklist

@cla-bot cla-bot bot added the cla:yes label Jan 3, 2023
@MichelleArk MichelleArk changed the base branch from main to feature/click-cli January 3, 2023 21:28
@MichelleArk MichelleArk marked this pull request as ready for review January 3, 2023 21:30
@MichelleArk MichelleArk requested a review from a team January 3, 2023 21:30
@MichelleArk MichelleArk requested review from a team as code owners January 3, 2023 21:30
@MichelleArk MichelleArk requested review from iknox-fa and gshank and removed request for gshank January 3, 2023 21:30
@MichelleArk MichelleArk closed this Jan 3, 2023
@MichelleArk MichelleArk reopened this Jan 3, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jan 3, 2023

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

Copy link
Contributor

@stu-k stu-k left a comment

Choose a reason for hiding this comment

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

Nothing stands out 👍

@gshank
Copy link
Contributor

gshank commented Jan 4, 2023

I suspect that MP_CONTEXT should not be in the Flags object. It's not really a flag, and it's not serializable. I had to jump through some hoops to prevent it from serializing a while back. I'm not sure where it would be best to put it. And it's another global thing...

@jtcohen6
Copy link
Contributor

jtcohen6 commented Jan 4, 2023

I suspect that MP_CONTEXT should not be in the Flags object. It's not really a flag, and it's not serializable. I had to jump through some hoops to prevent it from serializing a while back. I'm not sure where it would be best to put it. And it's another global thing...

@gerda MP_CONTEXT - gosh - there's some gnarliness here:

dbt-core/core/dbt/flags.py

Lines 120 to 126 in 8217ad4

def _get_context():
# TODO: change this back to use fork() on linux when we have made that safe
return multiprocessing.get_context("spawn")
# This is not a flag, it's a place to store the lock
MP_CONTEXT = _get_context()

Agree it really doesn't belong on the flags object / module. It's probably just there because that's where we've historically stuck a lot of our globals. It does seem to be used for locks within dbt-core (when things need to be thread-safe), and also within dbt-rpc.

That looks unrelated to the changes in this PR, though?

@gshank
Copy link
Contributor

gshank commented Jan 4, 2023

I tried looking through some of the test failures and in at least one case I saw an error about "unable to serialize SpawnContext", which is from that. The test output started disappearing though...

@MichelleArk
Copy link
Contributor Author

MichelleArk commented Jan 4, 2023

I tried looking through some of the test failures and in at least one case I saw an error about "unable to serialize SpawnContext", which is from that. The test output started disappearing though...

@gshank - Thank you for looking into the type TypeError: Object of type SpawnContext is not JSON serializable errors, and for the additional context @jtcohen6! Integration tests on the feature/click-cli branch are expected to fail since we started running integration tests through the new click framework which is under development and still missing large portions of its implementation. This issue does feel out of scope for this PR but I've opened an issue to track it as part of the API-ification epic: #6509.

Copy link
Contributor

@iknox-fa iknox-fa left a comment

Choose a reason for hiding this comment

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

LGTM!

@iknox-fa
Copy link
Contributor

iknox-fa commented Jan 6, 2023

I tried looking through some of the test failures and in at least one case I saw an error about "unable to serialize SpawnContext", which is from that. The test output started disappearing though...

@gshank - Thank you for looking into the type TypeError: Object of type SpawnContext is not JSON serializable errors, and for the additional context @jtcohen6! Integration tests on the feature/click-cli branch are expected to fail since we started running integration tests through the new click framework which is under development and still missing large portions of its implementation. This issue does feel out of scope for this PR but I've opened an issue to track it as part of the API-ification epic: #6509.

+100 from my POV. I think this one falls into the overarching de-globalization effort that @ChenyuLInx will be working on at some point as tech-lead of the "gap work" for dbt-server.

@MichelleArk MichelleArk force-pushed the CT-915/remove-unset-profile-config branch from 096231e to bc04906 Compare January 7, 2023 00:38
@MichelleArk MichelleArk merged commit 5b31cc4 into feature/click-cli Jan 7, 2023
@MichelleArk MichelleArk deleted the CT-915/remove-unset-profile-config branch January 7, 2023 01:12
This was referenced Feb 8, 2023
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-915] High Level API - Unset Profile Config
5 participants