You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/08/30 08:49:24 UTC
[GitHub] [spark] itholic opened a new pull request, #37724: [SPARK-40273][PYTHON][DOCS] Fix the documents "Contributing and Maintaining Type Hints".
itholic opened a new pull request, #37724:
URL: https://github.com/apache/spark/pull/37724
### What changes were proposed in this pull request?
This PR proposes to fix the [Contributing and Maintaining Type Hints](https://spark.apache.org/docs/latest/api/python/development/contributing.html#contributing-and-maintaining-type-hints) since the existing type hints in the stub files are all ported into inline type hints.
### Why are the changes needed?
We no longer use the stub files for type hinting, so we might need to change the documents as well.
### Does this PR introduce _any_ user-facing change?
Yes, the documentation change.
### How was this patch tested?
The existing documentation build should pass
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] itholic commented on pull request #37724: [SPARK-40273][PYTHON][DOCS] Fix the documents "Contributing and Maintaining Type Hints".
Posted by GitBox <gi...@apache.org>.
itholic commented on PR #37724:
URL: https://github.com/apache/spark/pull/37724#issuecomment-1232654297
@zero323 Let me address it. Thanks for the suggestion!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] itholic commented on a diff in pull request #37724: [SPARK-40273][PYTHON][DOCS] Fix the documents "Contributing and Maintaining Type Hints".
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #37724:
URL: https://github.com/apache/spark/pull/37724#discussion_r962436689
##########
python/docs/source/development/contributing.rst:
##########
@@ -189,9 +186,6 @@ Annotations should, when possible:
* Be compatible with the current stable MyPy release.
-
-Complex supporting type definitions, should be placed in dedicated ``_typing.pyi`` stubs. See for example `pyspark.sql._typing.pyi <https://github.com/apache/spark/blob/master/python/pyspark/sql/_typing.pyi>`_.
Review Comment:
Yeah, this is intentionally removed since I think we better not to mention about the stub files (pyi) in the guide.
Do you think it's good to keep this part as it is? (I have no strong opinion in either way, though)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #37724: [SPARK-40273][PYTHON][DOCS] Fix the documents "Contributing and Maintaining Type Hints".
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on PR #37724:
URL: https://github.com/apache/spark/pull/37724#issuecomment-1232313321
cc @zero323 FYI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zero323 closed pull request #37724: [SPARK-40273][PYTHON][DOCS] Fix the documents "Contributing and Maintaining Type Hints".
Posted by GitBox <gi...@apache.org>.
zero323 closed pull request #37724: [SPARK-40273][PYTHON][DOCS] Fix the documents "Contributing and Maintaining Type Hints".
URL: https://github.com/apache/spark/pull/37724
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zero323 commented on a diff in pull request #37724: [SPARK-40273][PYTHON][DOCS] Fix the documents "Contributing and Maintaining Type Hints".
Posted by GitBox <gi...@apache.org>.
zero323 commented on code in PR #37724:
URL: https://github.com/apache/spark/pull/37724#discussion_r961460456
##########
python/docs/source/development/contributing.rst:
##########
@@ -189,9 +186,6 @@ Annotations should, when possible:
* Be compatible with the current stable MyPy release.
-
-Complex supporting type definitions, should be placed in dedicated ``_typing.pyi`` stubs. See for example `pyspark.sql._typing.pyi <https://github.com/apache/spark/blob/master/python/pyspark/sql/_typing.pyi>`_.
Review Comment:
One more question ‒ do we intentionally remove this @itholic?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #37724: [SPARK-40273][PYTHON][DOCS] Fix the documents "Contributing and Maintaining Type Hints".
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #37724:
URL: https://github.com/apache/spark/pull/37724#discussion_r958287277
##########
python/docs/source/development/contributing.rst:
##########
@@ -155,18 +155,13 @@ Now, you can start developing and `running the tests <testing.rst>`_.
Contributing and Maintaining Type Hints
----------------------------------------
-PySpark type hints are provided using stub files, placed in the same directory as the annotated module, with exception to:
-
-* ``# type: ignore`` in modules which don't have their own stubs (tests, examples and non-public API).
-* pandas API on Spark (``pyspark.pandas`` package) where the type hints are inlined.
-
-As a rule of thumb, only public API is annotated.
+PySpark type hints are inlined, to take advantage of static type checking within the functions.
Review Comment:
I think we should keep "As a rule of thumb, only public API is annotated." I think it's not "within the functions" since it include parameters, etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] itholic commented on a diff in pull request #37724: [SPARK-40273][PYTHON][DOCS] Fix the documents "Contributing and Maintaining Type Hints".
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #37724:
URL: https://github.com/apache/spark/pull/37724#discussion_r964313406
##########
python/docs/source/development/contributing.rst:
##########
@@ -189,9 +186,6 @@ Annotations should, when possible:
* Be compatible with the current stable MyPy release.
-
-Complex supporting type definitions, should be placed in dedicated ``_typing.pyi`` stubs. See for example `pyspark.sql._typing.pyi <https://github.com/apache/spark/blob/master/python/pyspark/sql/_typing.pyi>`_.
Review Comment:
Got it. Let's keep this as it is for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zero323 commented on a diff in pull request #37724: [SPARK-40273][PYTHON][DOCS] Fix the documents "Contributing and Maintaining Type Hints".
Posted by GitBox <gi...@apache.org>.
zero323 commented on code in PR #37724:
URL: https://github.com/apache/spark/pull/37724#discussion_r963768068
##########
python/docs/source/development/contributing.rst:
##########
@@ -189,9 +186,6 @@ Annotations should, when possible:
* Be compatible with the current stable MyPy release.
-
-Complex supporting type definitions, should be placed in dedicated ``_typing.pyi`` stubs. See for example `pyspark.sql._typing.pyi <https://github.com/apache/spark/blob/master/python/pyspark/sql/_typing.pyi>`_.
Review Comment:
`_typing` modules are here to stay as they're fundamental part of our annotations so I don't think we should ignore them in the typing guide.
As of mentioning stubs explicitly I don't have strong opinion. We won't be able to switch to plain Python modules before the successor of 3.4 the earliest (we need to drop Python 3.7 to support `Protocol` and `Literal` if I recall correctly) assuming we won't use any new typing features and cyclic dependencies won't cause too much problems for migration. However, it is not necessary to mention that we use specifically use stubs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zero323 commented on pull request #37724: [SPARK-40273][PYTHON][DOCS] Fix the documents "Contributing and Maintaining Type Hints".
Posted by GitBox <gi...@apache.org>.
zero323 commented on PR #37724:
URL: https://github.com/apache/spark/pull/37724#issuecomment-1239144233
Merged to master.
Thanks everyone!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zero323 commented on pull request #37724: [SPARK-40273][PYTHON][DOCS] Fix the documents "Contributing and Maintaining Type Hints".
Posted by GitBox <gi...@apache.org>.
zero323 commented on PR #37724:
URL: https://github.com/apache/spark/pull/37724#issuecomment-1232585621
I guess it's OK, though:
- Explicit mention of inling might be a bit misleading ‒ we still have quite a few stubs which are not going anywhere ( maybe we can just say "PySpark provides type hints to take advantage of static type checking.")
- Notes about ignored parts are still, to large extent, applicable. For example, we ignore untyped defs in tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on a diff in pull request #37724: [SPARK-40273][PYTHON][DOCS] Fix the documents "Contributing and Maintaining Type Hints".
Posted by GitBox <gi...@apache.org>.
srowen commented on code in PR #37724:
URL: https://github.com/apache/spark/pull/37724#discussion_r962165446
##########
python/docs/source/development/contributing.rst:
##########
@@ -189,9 +186,6 @@ Annotations should, when possible:
* Be compatible with the current stable MyPy release.
-
-Complex supporting type definitions, should be placed in dedicated ``_typing.pyi`` stubs. See for example `pyspark.sql._typing.pyi <https://github.com/apache/spark/blob/master/python/pyspark/sql/_typing.pyi>`_.
Review Comment:
Checking in on this last question @itholic
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org