implement metrics counter feature tracking for NADE #1597
Conversation
| TEMPLATES_PROCESSOR = f"{_FEATURES_PREFIX}.templates_processor" | ||
| SQL_TEMPLATES = f"{_FEATURES_PREFIX}.sql_templates" | ||
| PDF_TEMPLATES = f"{_FEATURES_PREFIX}.pdf_templates" | ||
| SNOWPARK_PROCESSOR = f"{_FEATURES_PREFIX}.{_APP_PREFIX}.snowpark_processor" |
There was a problem hiding this comment.
I am wondering if we should switch these, so app comes first, so we can separate these easily? @sfc-gh-bdufour , @sfc-gh-turbaszek wdyt?
There was a problem hiding this comment.
What's here makes sense to me, I don't think I understand the suggestion. Can you explain?
| @@ -340,6 +342,18 @@ def render_definition_template( | |||
| project_context[CONTEXT_KEY]["env"] = environment_overrides | |||
| return ProjectProperties(project_definition, project_context) | |||
|
|
|||
| has_user_referenced_vars = ( | |||
There was a problem hiding this comment.
can we put this whole new logic into a function?
There was a problem hiding this comment.
I also refactored the usage here with this function, let me know if this works for you!
| @@ -340,6 +342,18 @@ def render_definition_template( | |||
| project_context[CONTEXT_KEY]["env"] = environment_overrides | |||
| return ProjectProperties(project_definition, project_context) | |||
|
|
|||
| has_user_referenced_vars = ( | |||
| len(_get_referenced_vars_in_definition(template_env, definition)) > 0 | |||
There was a problem hiding this comment.
is there a risk/side effect from calling this?
There was a problem hiding this comment.
I went through the called functions and i do not believe there should be any, unless self._jinja_env.parse has side effects?
There was a problem hiding this comment.
it could throw error, but I guess it will throw them later anyways
|
I think this will require a fair amount of rework before being checked in, I'll do a full pass tomorrow though. Just to clarify, this is a not a target for inclusion in the 3.0 release (cc @sfc-gh-turbaszek) |
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| from snowflake.snowpark.functions import sproc, udf |
There was a problem hiding this comment.
it was an explicit choice to do this in E2E tests and not integ. That's because it didn't seem right to rely on the fact that the CLI is currently bundling the snowpark lib, or which version it's using. It's a bit more of an implementation detail IMHO. In E2E though, we get to install the snowpark lib in the test environment, so that's a lot more robust.
| TEMPLATES_PROCESSOR = f"{_FEATURES_PREFIX}.templates_processor" | ||
| SQL_TEMPLATES = f"{_FEATURES_PREFIX}.sql_templates" | ||
| PDF_TEMPLATES = f"{_FEATURES_PREFIX}.pdf_templates" | ||
| SNOWPARK_PROCESSOR = f"{_FEATURES_PREFIX}.{_APP_PREFIX}.snowpark_processor" |
There was a problem hiding this comment.
What's here makes sense to me, I don't think I understand the suggestion. Can you explain?
| def __eq__(self, other): | ||
| if isinstance(other, CLIMetrics): | ||
| return self._counters == other._counters | ||
| return False |
There was a problem hiding this comment.
why do we need this?
| def set_counter(self, name: str, value: int) -> None: | ||
| self._counters[name] = value | ||
|
|
||
| def add_counter(self, name: str, value: int) -> None: |
There was a problem hiding this comment.
nit: increment_counter
nit: optional value = 1
Pre-review checklist
Changes description