SNOW-1342243: Call getOrCreate at package import time so that sproc user does not need to

[sfc-gh-helmeleegy]

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

   Fixes SNOW-1342243

2. Fill out the following pre-review checklist:

   • I am adding a new automated test(s) to verify correctness of my new code
     • If this test skips Local Testing mode, I'm requesting review from @snowflakedb/local-testing
   • I am adding new logging messages
   • I am adding a new telemetry message
   • I am adding new credentials
   • I am adding a new dependency
   • If this is a new feature/behavior, I'm adding the Local Testing parity changes.

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

   Call getOrCreate at package import time so that sproc user does not need to.

[sfc-gh-jrose]

This change seems dangerous to me. Here are some scenarios that might break:

• What happens when a user does not have credentials configured before importing this package.
• What happens if the user ignores the default session and creates their own, then calls something that references get_active_session?
• What happens if a user imports this package while using local testing mode?

[sfc-gh-yzou]

@sfc-gh-helmeleegy for snowpark pandas, we by design to ask the user to explicitly set up the session to stay consistent with sowpark usage, and we do not require customer to provide connection files , so getOrCreate() could fail if user does not have a connection file.

With stored sproc, i thought the session object is automatically created, and snowaprk pandas have the capablity to get the current active session for usage. For some reason, that seems not the case. @sfc-gh-sfan why it is happening in such way?

[sfc-gh-yzou]

actually @sfc-gh-sfan I have a question, do you have a code pointer on how the session creation is handled in stored sproc today? One thing i am not sure here is if that getOrCreate will always work.

[sfc-gh-helmeleegy]

After making the session argument optional in the proc's handler, the session object was not being created anymore in the case when the session argument was not specified. That's because it was not being passed along with the other arguments to the handler anymore.

@sfc-gh-zyao has a draft PR for pre-creating session objects even for session-less handlers, which seems promising. Until this becomes available, we will need to either pass the session argument, or create it at the sproc side using getOrCreate. Currently, it's left to the user to create it inside the body of the handler.

This PR attempts to automatically create the session object at package import time (only for sprocs) so that users do not need to worry about creating it themselves.

[sfc-gh-yzou]

Yeah, i think the direction @sfc-gh-zyao is the right direction. Our original intention is to make the argument optional, not making the session object creation optional. I was talking to @sfc-gh-zyao , he mentioned he will work on this to get the fix in soon. I think we can just wait for this fix, instead of adding the workaround here, since it also took long time for the package to be released in stored sproc also.

[sfc-gh-mvashishtha]

I thought the goal was to test that session is not required.

[sfc-gh-helmeleegy]

Before this PR, the Snowpark python API would still require having a session argument for the sproc's handler. I updated the PR to allow for optional session argument even when using the Snowpark python API and added tests for both cases (with and without the session argument).

@sfc-gh-sfan would be great if you can confirm that the update to Snowpark python does make sense.

[sfc-gh-mvashishtha]

If it's possible to create multiple sessions in a stored procedure, then there's an edge case where the user creates multiple Snowpark sessions in the stored procedure and then imports snowpark pandas. Before this change, that would work, but now, they'll get an error.

[sfc-gh-aalam]

I don't understand the need for this. Inside a python stored proc environment, we already have access to a session object. Why can't we use that?

[sfc-gh-helmeleegy]

@sfc-gh-aalam As pointed out in another thread here. Since we made the session argument optional (at the server/xp side), the session object was not being created anymore unless it was explicitly specified in the sproc's handler. Otherwise, it won't be created. This draft PR should make sure it's created in both cases.

@sfc-gh-mvashishtha I guess if we want, we can try-catch this scenario and suppress that error if it was raised - to make sure we keep the current behavior?

[sfc-gh-mvashishtha]

Should we add a TODO to stop doing this once Snowpark always creates the session automatically, with a link to the jira ticket for that?

[sfc-gh-yzou]

I am not aware that snowpark has plan to create the session automatically. cc @sfc-gh-jdu do we have any plan for this?

The problem here is more for stored sproc, in stored sproc, user is suppose to use the session created by stored sproc which uses specific connection configurations, because the one used by users are not usable within stored sproc.
Originally, we thought stored sproc will always create the session, but seems it is only creating the session when the session parameter passed. The fix/workaround here seems make sure if snowpark pandas is used in stored sproc, make sure we create the session.
However, since session parameter is a required parameter for stored sproc before, it seems it is almost always creating a session object. I think we can always create a session object in snowpark python stored sproc, i don't see a reason why user will not use a session object with snowpark python stored sproc. cc @sfc-gh-sfan

[sfc-gh-helmeleegy]

@sfc-gh-yzou Please refer to the other conversation threads.

[sfc-gh-helmeleegy]

This change seems dangerous to me. Here are some scenarios that might break:

• What happens when a user does not have credentials configured before importing this package.
• What happens if the user ignores the default session and creates their own, then calls something that references get_active_session?
• What happens if a user imports this package while using local testing mode?

These are all good points @sfc-gh-jrose. Do you think that they will be addressed after this other PR is merged? In this case, calling getOrCreate will actually get rather than create. Also, is there a way to detect if we are in a local testing mode, such that we can skip this workaround?

[sfc-gh-jrose]

These are all good points @sfc-gh-jrose. Do you think that they will be addressed after this other PR is merged? In this case, calling getOrCreate will actually get rather than create. Also, is there a way to detect if we are in a local testing mode, such that we can skip this workaround?

With the code now gated by is_in_stored_procedure I don't think local testing is a concern.