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

Fix default config reading import #1042

Merged
merged 2 commits into from
Sep 8, 2023
Merged

Fix default config reading import #1042

merged 2 commits into from
Sep 8, 2023

Conversation

sfc-gh-jdu
Copy link
Collaborator

The original import won't work in stored procedure, so we need to import it inside the create function

@sfc-gh-jdu sfc-gh-jdu requested a review from a team as a code owner September 7, 2023 23:37
@sfc-gh-jdu sfc-gh-jdu added the NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md label Sep 7, 2023
Copy link
Collaborator

@sfc-gh-sfan sfc-gh-sfan left a comment

Choose a reason for hiding this comment

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

We might want to add this function to sp connector even if it is just a no-op to reduce the stored proc check we have in code. Can you create a ticket to track that?

@@ -21,7 +21,6 @@
import pkg_resources

from snowflake.connector import ProgrammingError, SnowflakeConnection
from snowflake.connector.config_manager import _get_default_connection_params
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably can do is_in_stored_procedure() check for the import? And for safety, also add the same check in the if clause before calling the function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

@sfc-gh-sfan sfc-gh-sfan left a comment

Choose a reason for hiding this comment

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

Sorry does not mean to approve yet

@sfc-gh-jdu sfc-gh-jdu force-pushed the jdu-fix-default-config branch from c98b1b0 to 3225e9e Compare September 8, 2023 00:05
@sfc-gh-jdu
Copy link
Collaborator Author

We might want to add this function to sp connector even if it is just a no-op to reduce the stored proc check we have in code. Can you create a ticket to track that?

sure, created https://snowflakecomputing.atlassian.net/browse/SNOW-911000

@sfc-gh-jdu sfc-gh-jdu merged commit df013e5 into main Sep 8, 2023
50 checks passed
@sfc-gh-jdu sfc-gh-jdu deleted the jdu-fix-default-config branch September 8, 2023 03:26
@github-actions github-actions bot locked and limited conversation to collaborators Sep 8, 2023
@sfc-gh-aalam
Copy link
Contributor

2023-09-08 11:16:32     from snowflake.snowpark.session import Session
2023-09-08 11:16:32   File "/home/jenkins/workspace/AnacondaPackageBuilder-x86/conda-bld/snowflake-snowpark-python_1694196935262/_test_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_pl/lib/python3.8/site-packages/snowflake/snowpark/session.py", line 154, in <module>
2023-09-08 11:16:32     from snowflake.connector.config_manager import _get_default_connection_params
2023-09-08 11:16:32 ImportError: cannot import name '_get_default_connection_params' from 'snowflake.connector.config_manager' (/home/jenkins/workspace/AnacondaPackageBuilder-x86/conda-bld/snowflake-snowpark-python_1694196935262/_test_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_pl/lib/python3.8/site-packages/snowflake/connector/config_manager.py)

This is still breaking https://ci-dev-142.int.snowflakecomputing.com/job/AnacondaPackageBuilder-x86/

@sfc-gh-jdu can you look into this and verify if our precommit test is happy?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants