This repository has been archived by the owner on Nov 22, 2022. It is now read-only.
-
-
Notifications
You must be signed in to change notification settings - Fork 37
Add missing methods #464
Closed
Closed
Add missing methods #464
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
1821a3f
Add missing methods
Fokko 1bcfdde
Make Black happy
Fokko aa508b0
Thanks for the feedback!
Fokko f24c478
Merge branch 'master' of github.com:zero323/pyspark-stubs into fd-mis…
Fokko de1da71
Install both numpy and pandas
Fokko c4237dc
Remove Python2 relics
Fokko 70c2bf7
Merge branch 'master' into fd-missing-annotations
Fokko 3f0d18e
Merge branch 'master' into fd-missing-annotations
Fokko 6d0a90b
Merge branch 'master' into fd-missing-annotations
zero323 5e394c9
Add missing annotation
Fokko 44f6aae
Merge branch 'master' into fd-missing-annotations
zero323 6b22da8
Merge branch 'master' into fd-missing-annotations
zero323 cbfcbf4
Merge branch 'master' into fd-missing-annotations
zero323 db3c5e7
Merge branch 'master' into fd-missing-annotations
zero323 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we annotate this, could we be more precise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it holds the Accumulators, where the int is a key: https://github.com/apache/spark/blob/7deb67c28f948cca4e768317ade6d68d2534408f/python/pyspark/context.py#L902
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering ‒ what is the reason for including this? I am not strictly against this, but I haven't seen any need for this one so far and in general we seem to lean towards excluding internal APIs, unless strictly necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
_accumulatorRegistry
is used by other files, therefore we have to include it. Otherwise, mypy will complain that it can't find the dict.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Could your provide some details on the test setup? Haven't seen any mypy failures in apache/spark#29591 (still work in progress, though I don't expect it will make a huge impact on type checks).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll whip up an example first thing tomorrow morning!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Have a good night.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be easily be reproducable on your machine as well. I'm working on my branch of apache/spark#29180:
Right now everything passes:
If we remove the
_accumulatorRegistry
:Then it fails:
It looks like the
_accumulatorRegistry
is private, since it starts with a_
, but it is actually being imported inworker.py
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I see... This won't happen on my side, as
worker.py
has dynamic stub.There are quite a few of these imports all over the place. In general I tend to use specific
ignores
for these, as they're not user facing API.If you don't mind, I'll keep this open for now and finish syncing things for SPARK-32714, as it might require some further discussion about the scope of annotations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, let me know if I can help somewhere