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.

Sign up for GitHub

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

Jump to bottom

Alter closing sessions inside a stored procedure to write a warning instead of throwing an error #1263

Merged
merged 18 commits into from
Mar 21, 2024
Merged

Alter closing sessions inside a stored procedure to write a warning instead of throwing an error #1263

merged 18 commits into from
Mar 21, 2024
+52 −13

Conversation

zaramzamzam
Copy link
Contributor

  1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes SNOW-1062439: Automatically closing sessions and connections using Python with leads to errors when deploying to stored procedures #1259

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am adding new credentials
    • I am adding a new dependency

  3. Please describe how your code solves the related issue.

Instead of throwing an error message when attempting to close a session a warning is written informing that closing a session inside a stored procedure is a no-op.
Afterwards the functionality to close sessions is ignored by returning from the function directly after writing a warning.

@zaramzamzam
add regression test for old behavior
0553eba 
- this should fail once closing sessions in stored procedures is a noop
@zaramzamzam
write a warning when closing a session inside a stored procedure
8b11568 
- instead of throwing an error a warning is written when closing a session inside a
  stored procedure
@zaramzamzam
remove skip condition for session closing integration test when insid…
69ca98b 
…e a stored procedure

- skip should not be needed anymore due to the closing of a session being a no-op
  when inside a stored procedure
@zaramzamzam
rename test functions
56459d8
@zaramzamzam
add no-op functionality when closing sessions in stored procedure
e371bad
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 21, 2024 •
edited
Loading

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@zaramzamzam
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@zaramzamzam zaramzamzam marked this pull request as ready for review February 21, 2024 12:27
@zaramzamzam zaramzamzam requested a review from a team as a code owner February 21, 2024 12:27
@zaramzamzam zaramzamzam marked this pull request as draft February 21, 2024 12:28
@zaramzamzam zaramzamzam marked this pull request as ready for review February 21, 2024 12:29
@zaramzamzam
update skif if reason
72fef95 
- test cannot be run because creating a session in stored procedures is not allowed
sfc-gh-aalam
sfc-gh-aalam requested changes Feb 21, 2024
View reviewed changes
tests/integ/test_session.py Show resolved Hide resolved
src/snowflake/snowpark/session.py Outdated Show resolved Hide resolved
tests/unit/test_session.py Outdated Show resolved Hide resolved
tests/unit/test_session.py Outdated Show resolved Hide resolved
tests/unit/test_session.py Outdated Show resolved Hide resolved
@zaramzamzam
Copy link
Contributor Author

Thanks for the review!
I reran pre-commit to ensure formatting is consistent and combined the log level tests.-

sfc-gh-aalam
sfc-gh-aalam reviewed Feb 23, 2024
View reviewed changes
Copy link
Contributor

@sfc-gh-aalam sfc-gh-aalam left a comment

Choose a reason for hiding this comment

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

Overall change looks good. just one last comment to remove the skip. I'll run tests from my end to verify that removing the skip passes all tests

@zaramzamzam
Copy link
Contributor Author

I fixed the failing test. The integration test was stilling testing for the old behavior.

@sfc-gh-aalam
Copy link
Contributor

SP tests: https://ci-dev-142.int.snowflakecomputing.com/job/PythonStoredProcBuildPrecommitTest/

@sfc-gh-aalam
Copy link
Contributor

responding a little late on this thread because was discussing with the team about the result of internal tests we ran earlier

SP tests: https://ci-dev-142.int.snowflakecomputing.com/job/PythonStoredProcBuildPrecommitTest/

All tests passed except for one which is testing log messages received during the life-cycle of a stored proc. We see extra log lines in that test due to session.close() being called during __exit__ and _close_session_atexit

I made these changes myself here and made sure that all tests have passed.
The final request from you would replicate this and we can move ahead and merge this change.

@zaramzamzam
Copy link
Contributor Author

Thanks for the input.
I will incorporate this tomorrow.

@sfc-gh-aalam sfc-gh-aalam requested a review from sfc-gh-sfan March 5, 2024 19:30
@sfc-gh-aalam
Copy link
Contributor

There was a recent change on the server side. can you rebase and pull latest changes from main and the tests will pass.

@zaramzamzam
Copy link
Contributor Author

There was a recent change on the server side. can you rebase and pull latest changes from main and the tests will pass.

The branch is now up to date with the main.

@zaramzamzam zaramzamzam requested a review from sfc-gh-aalam March 6, 2024 11:20
@sfc-gh-aalam sfc-gh-aalam merged commit 11eb2af into snowflakedb:main Mar 21, 2024
65 of 112 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sfc-gh-aalam sfc-gh-aalam sfc-gh-aalam approved these changes

@sfc-gh-sfan sfc-gh-sfan sfc-gh-sfan approved these changes

@sfc-gh-jdu sfc-gh-jdu Awaiting requested review from sfc-gh-jdu sfc-gh-jdu is a code owner automatically assigned from snowflakedb/snowpark-python-api-reviewers

@sfc-gh-yuwang sfc-gh-yuwang Awaiting requested review from sfc-gh-yuwang sfc-gh-yuwang is a code owner automatically assigned from snowflakedb/snowpark-python-api-reviewers

@sfc-gh-jrose sfc-gh-jrose Awaiting requested review from sfc-gh-jrose sfc-gh-jrose is a code owner automatically assigned from snowflakedb/snowpark-python-api-reviewers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@zaramzamzam @sfc-gh-aalam @sfc-gh-sfan