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

Temporarily disable log_telemetry_imported_packages #52

Merged
merged 4 commits into from
Jul 19, 2023

Conversation

jrbourbeau
Copy link
Member

It so happens we can just disable the non-thread safe functionality in the Snowflake connector highlighted in #51.

I think turning it off by default (with a meaningful error message when a users explicit wants it on) as a temporary workaround for snowflakedb/snowflake-connector-python#1648 makes sense.

@fjetter do you have bandwidth to review?

@jrbourbeau
Copy link
Member Author

CI has run here 4 times successfully. I'm probably going to merge this in later today if nobody objects. Prohibiting this particular config option seems like it will have little impact of real-world users (the config option seems rather exotic?) while fixing the multithreading issue we've seen on main

Copy link
Contributor

@j-bennet j-bennet left a comment

Choose a reason for hiding this comment

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

FWIW, I think it's a good idea (sorry I'm not Florian). We can keep an eye on the upstream issue and remove the ValueError when that's fixed.

Copy link
Member Author

@jrbourbeau jrbourbeau 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 feedback @j-bennet, I appreciate it. Let's merge this in and track the upstream issue like you said (I've subscribed to it on GH so will see when progress is made)

@jrbourbeau jrbourbeau merged commit d60a9af into main Jul 19, 2023
@jrbourbeau jrbourbeau deleted the dict-workaround branch July 19, 2023 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants