SNOW-1791994 IR: Generate typed protobuf Python code #2593
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sfc-gh-azhan
added
NO-CHANGELOG-UPDATES
sfc-gh-azhan
force-pushed
the
azhan-proto-type-1791994
branch
8 times, most recently
from
9f796a2
to
6d9ab25
Compare
sfc-gh-azhan
force-pushed
the
azhan-proto-type-1791994
branch
4 times, most recently
from
c7e0af0
to
6e576e7
Compare
sfc-gh-azhan
changed the title
sfc-gh-azhan
force-pushed
the
azhan-proto-type-1791994
branch
from
6e576e7
to
3cf6597
Compare
sfc-gh-azhan
force-pushed
the
azhan-proto-type-1791994
branch
from
3cf6597
to
6fb5f1b
Compare
sfc-gh-azhan
commented
Nov 11, 2024
| run: | | ||
| make clean | ||
| make html SPHINXOPTS="-W --keep-going" | ||
| run: python -m tox -e docs |
There was a problem hiding this comment.
just reuse the same code in tox
| @@ -91,6 +91,7 @@ repos: | |||
| - id: mypy | |||
| files: > | |||
| (?x)^( | |||
| src/snowflake/snowpark/_internal/ast/.*\.py| | |||
There was a problem hiding this comment.
cover all ast code in mypy type check
| @@ -80,14 +80,14 @@ def __init__(self) -> None: # pragma: no cover | |||
| self.symbols: Optional[Union[str, List[str]]] = None | |||
|
|
|||
| # TODO(SNOW-1491199) - This method is not covered by tests until the end of phase 0. Drop the pragma when it is covered. | |||
| def visit_Assign(self, node) -> None: # pragma: no cover | |||
| def visit_Assign(self, node: ast.Assign) -> None: # pragma: no cover | |||
There was a problem hiding this comment.
we should fix those once our code is in.
sfc-gh-azhan
requested review from
sfc-gh-nkrishna,
sfc-gh-jdu,
sfc-gh-yixie and
sfc-gh-aling
sfc-gh-azhan
changed the title
| ): | ||
| stmt = func._ast_stmt | ||
| fn_expr = expr.fn.indirect_table_fn_id_ref | ||
| stmt = func._ast_stmt # type: ignore[union-attr] # TODO(SNOW-1491199) # Item "str" of "Union[str, list[str], TableFunctionCall, Callable[..., Any]]" has no attribute "_ast_stmt", Item "list[str]" of "Union[str, list[str], TableFunctionCall, Callable[..., Any]]" has no attribute "_ast_stmt", Item "TableFunctionCall" of "Union[str, list[str], TableFunctionCall, Callable[..., Any]]" has no attribute "_ast_stmt", Item "function" of "Union[str, list[str], TableFunctionCall, Callable[..., Any]]" has no attribute "_ast_stmt" |
There was a problem hiding this comment.
I assume the comments are machine-generated. Does any particular linter or annotation checker require the full message? Looks pretty cumbersome.
There was a problem hiding this comment.
Yes, it contains three parts:
- jira no. to track
- the type ignore
- the error message
It's probably verbose for the error message but I try to capture them so when we fix them later, we can easily know what was the issue before.
|
Thank you for setting this up! Generally looks good. I did not review |
| @@ -57,6 +57,7 @@ | |||
| "pytest-assume", # sql counter check | |||
| "decorator", # sql counter check | |||
| "protoc-wheel-0==21.1", # Protocol buffer compiler, for Snowpark IR | |||
| "mypy-protobuf", # used in generating typed Python code from protobuf for Snowpark IR | |||
There was a problem hiding this comment.
I assume this has no licensing or security concerns, either. Should this require a specific minimum version?
There was a problem hiding this comment.
Yes, this is development requirements so should be fine.
sfc-gh-oplaton
approved these changes
Nov 11, 2024
sfc-gh-aalam
approved these changes
Nov 11, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-1791994
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.
This PR generates typed Python code from protobuf and make sure CI check types in precommit.
Copilot summary:
This pull request includes several changes to improve type checking, streamline the build process, and enhance code quality. The most important changes include the addition of
mypy-protobuffor generating typed Python code from protobuf, updates to the build and pre-commit configurations, and improvements to type annotations across various files.Build and Dependency Management:
mypy-protobufinstallation in.github/scripts/install_protoc.shandscripts/jenkins_regress.shto generate typed Python code from protobuf. [1] [2] [3].github/workflows/precommit.ymlto installtoxand streamline the documentation build process usingtox.setup.pyto includemypy-protobufas a dependency and updated the protobuf generation command to use--mypy_out. [1] [2] [3]Pre-commit Configuration:
mypycheck in.pre-commit-config.yaml. [1] [2]Code Quality and Type Annotations:
src/snowflake/snowpark/_internal/ast/batch.pyby specifying return types and dictionary types. [1] [2] [3]src/snowflake/snowpark/_internal/ast/utils.pyby adding# type: ignorecomments and TODOs for better handling of type issues. [1] [2] [3] [4] [5] [6]Tested Jenkins jobs: