SNOW-1527717: Add profiler snowpark API #2252
Conversation
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
sfc-gh-yuwang
requested review from
sfc-gh-enie,
sfc-gh-jfreeberg and
sfc-gh-yixie
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
src/snowflake/snowpark/profiler.py
Outdated
| self.stage = stage | ||
| self.active_profiler = active_profiler | ||
| self.modules_to_register = [] | ||
| self.register_modules_sql = "" | ||
| self.set_targeted_stage_sql = "" | ||
| self.enable_profiler_sql = "" | ||
| self.disable_profiler_sql = "" | ||
| self.set_active_profiler_sql = "" | ||
| self.pattern = r"WITH\s+.*?\s+AS\s+PROCEDURE\s+.*?\s+CALL\s+.*" | ||
| self.session = session | ||
| self._prepare_sql() | ||
| self.query_history = None |
There was a problem hiding this comment.
do we want those member variable to be public? if not we can add "_" (self._stage) in front of them to make them private
src/snowflake/snowpark/profiler.py
Outdated
| Registered nodules will be overwritten by this function, | ||
| use this function with an empty string will remove registered modules. | ||
| Args: | ||
| modules: List of names of stored procedures. |
There was a problem hiding this comment.
is modules a specific term used in profiler? I find it conceptually hard to link this to the name of stored procedures..
| self, | ||
| stage: Optional[str] = "", | ||
| active_profiler: Optional[str] = "LINE", | ||
| session: Optional["snowflake.snowpark.Session"] = None, |
There was a problem hiding this comment.
what should customer expect to happen if session is None?
There was a problem hiding this comment.
session is required for profiler to work.
There is two way to use profiler as described in the design doc.
In context manager, session needed to be provided when create the context manager.
In profiler class, session is set when registering profiler.
This is why I make it optional here.
| == 0 | ||
| ): | ||
| self.session.sql( | ||
| f"create temp stage if not exists {self.stage} FILE_FORMAT = (RECORD_DELIMITER = NONE FIELD_DELIMITER = NONE )" |
There was a problem hiding this comment.
temp stage gets dropped at the end of the session which means we might lose all the profiler data.
when are users going to access the file, after the session or during the session?
There was a problem hiding this comment.
If user want to access it after the session, user would want to use a permanent stage they created.
when user give a stage name that does not exist, we expect user want to create a temp stage.
src/snowflake/snowpark/profiler.py
Outdated
| def enable_profiler(self): | ||
| """ | ||
| Enable profiler. Profiles will be generated until profiler is disabled. | ||
| """ | ||
| self.session.sql(self.enable_profiler_sql).collect() | ||
|
|
||
| def disable_profiler(self): |
There was a problem hiding this comment.
nit: I think the names of these two could be simplified as disable and enable:
| def enable_profiler(self): | |
| """ | |
| Enable profiler. Profiles will be generated until profiler is disabled. | |
| """ | |
| self.session.sql(self.enable_profiler_sql).collect() | |
| def disable_profiler(self): | |
| def enable(self): | |
| """ | |
| Enable profiler. Profiles will be generated until profiler is disabled. | |
| """ | |
| self.session.sql(self.enable_profiler_sql).collect() | |
| def disable(self): |
| def register_profiler(self, profiler: Profiler): | ||
| """Register a profiler to current session, all action are actually executed during this function""" | ||
| self.profiler = profiler | ||
| self.profiler.session = self |
There was a problem hiding this comment.
if we pass a profiler which is created a different session, is the overwriting here on purpose?
or another way to ask the question is should one profiler only be linked with one session?
There was a problem hiding this comment.
That's a good question, I think one profiler link to only one session is a good idea since the new session could have different setting.
|
Here is a link to the design doc: https://docs.google.com/document/d/1NCA7tHjZJY7kCmRPm8JUeQSAYmx7MtlOI3xyy0AgQmc/edit?usp=sharing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-NNNNNNN
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
Please write a short description of how your code change solves the related issue.