Skip to content

Conversation

@zero323
Copy link
Member

@zero323 zero323 commented Nov 16, 2020

What changes were proposed in this pull request?

This pull request:

These flags are enabled only for public API and disabled for tests and internal modules.

Additionally, these PR fixes missing annotations.

Why are the changes needed?

Primary reason to propose this changes is to use standard configuration as used by typeshed project. This will allow us to be more strict, especially when interacting with JVM code. See for example #29122 (review)

Additionally, it will allow us to detect cases where annotations have unintentionally omitted.

Does this PR introduce any user-facing change?

Annotations only.

How was this patch tested?

dev/lint-python.

@zero323
Copy link
Member Author

zero323 commented Nov 16, 2020

FYI @Fokko @HyukjinKwon

Still work in progress, any suggestions about desired flags (I considered show_error_codes, for clarity, and disallow_any_generics).

@HyukjinKwon
Copy link
Member

My only concern is the maintenance cost for being strict. If it doesn't add non-trivial overhead, I am good to be more strict on the type hints.

@zero323
Copy link
Member Author

zero323 commented Nov 16, 2020

My only concern is the maintenance cost for being strict. If it doesn't add non-trivial overhead, I am good to be more strict on the type hints.

For the first two options - we are already covered, as these were used in pyspark-stubs.

The third one required some fixes, but these are mostly things that should be there. In all other cases, we can either ignore the whole module (as with tests and such) or specific lines (for example I considered ignoring callJavaFunc as these annotations are hardly useful and functions are not exactly public).

So overall, I don't think there's much complexity added and using these, gives us more confidence that annotations are in a good shape.

@SparkQA
Copy link

SparkQA commented Nov 16, 2020

Test build #131122 has finished for PR 30382 at commit 4de37b2.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Nov 16, 2020

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35725/

@SparkQA
Copy link

SparkQA commented Nov 16, 2020

Test build #131124 has finished for PR 30382 at commit c42fe59.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Nov 16, 2020

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35725/

@SparkQA
Copy link

SparkQA commented Nov 16, 2020

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35727/

@SparkQA
Copy link

SparkQA commented Nov 16, 2020

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35727/

@SparkQA
Copy link

SparkQA commented Nov 22, 2020

Test build #131515 has finished for PR 30382 at commit 426c7bb.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Nov 23, 2020

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36119/

@SparkQA
Copy link

SparkQA commented Nov 23, 2020

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36119/

@HyukjinKwon
Copy link
Member

@zero323, let me know when you think this is ready. Looks fine to me.

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@SparkQA
Copy link

SparkQA commented Nov 23, 2020

Test build #131547 has finished for PR 30382 at commit 9182a98.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class PipelineReader(MLReader[Pipeline]):
  • class PipelineModelReader(MLReader[PipelineModel]):

@SparkQA
Copy link

SparkQA commented Nov 23, 2020

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36150/

@zero323 zero323 marked this pull request as ready for review November 23, 2020 13:08
@zero323
Copy link
Member Author

zero323 commented Nov 23, 2020

@zero323, let me know when you think this is ready. Looks fine to me.

Thanks @HyukjinKwon. I went over all violations of disallow_any_generics and these are mostly intended (little or no additional value gained by addressing these). So I think it ready to review.

Looks good!

Thanks for reviewing @Fokko!

@SparkQA
Copy link

SparkQA commented Nov 23, 2020

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36150/

@SparkQA
Copy link

SparkQA commented Nov 24, 2020

Test build #131599 has finished for PR 30382 at commit 9182a98.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class PipelineReader(MLReader[Pipeline]):
  • class PipelineModelReader(MLReader[PipelineModel]):

@HyukjinKwon
Copy link
Member

retest this please

@SparkQA
Copy link

SparkQA commented Nov 24, 2020

Test build #131614 has finished for PR 30382 at commit 9182a98.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class PipelineReader(MLReader[Pipeline]):
  • class PipelineModelReader(MLReader[PipelineModel]):

@HyukjinKwon
Copy link
Member

Merged to master.

@zero323
Copy link
Member Author

zero323 commented Nov 25, 2020

Thanks @Fokko and @HyukjinKwon!

@zero323 zero323 deleted the SPARK-33457 branch November 25, 2020 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants