SONARPY-1791: Typeshed add Scikit-learn stubs - #1778
Conversation
999f504 to
5531955
Compare
5531955 to
bcf46fa
Compare
ghislainpiot
left a comment
There was a problem hiding this comment.
Good work on a pretty sensitive part of the project. Other than the instability, I think it is mostly good !
There was a problem hiding this comment.
It works, but it is quite convoluted. If you do the steps incorrectly, you can get stuck in a sparse checkout and it is not obvious how to fix it.
There was a problem hiding this comment.
I know it is not perfect sadly there is no easier way. I think we could maybe automate the process, in a maven task or small script. But I think this could be a separate ticket/PR
There was a problem hiding this comment.
If I build the stubs on my machine, I get instability on the protobuf. We can discuss it offline
There was a problem hiding this comment.
We should probably avoid reformatting files that didn't change
There was a problem hiding this comment.
I think reformating files is okay, as we're in a somewhat inconsistent state right now (for a long time the produce json had a special format and it's hasn't been that long since we product valid json), and I personally prefer seeing formatting-only changes rather than to see mixed formatting and functional changes when this file is actually impacted.
However, I suggest having explicit formatting-only commits when this happens so that actual ruling changes can be reviewed in a focused way.
There was a problem hiding this comment.
Checksums don't match on my environment, I will retry it anyway with a fresh one
There was a problem hiding this comment.
Not sure if the formatting changes are really necessary, but it does look better
There was a problem hiding this comment.
Is that intentional or a remnant ?
There was a problem hiding this comment.
No good catch this is an error during merge!
There was a problem hiding this comment.
Nice, should we have a ticket or something in place to solve that ?
There was a problem hiding this comment.
I don't think we need to fix this as they are all import error as mypy tries to solve everything, even non relevant imports.
c623d96 to
915063b
Compare
Fix incorrect filter on typshed serializer Added new stubs to Typeshed symbols
915063b to
ee37a66
Compare
guillaume-dequenne
left a comment
There was a problem hiding this comment.
I left mostly feedback about how I'd prefer the PR to be prepared for review (cleaning commit history and splitting functional from non functional changes) but the changes themselves look good to me.
Just a few points:
- I'd recommend adding some tests for the logger.
- I'd suggest considering using a fork of the Microsoft stubs rather than the
sparse-checkout, but this could be done later. - I'm wondering about the priority/way we split stubs from different sources, but no strong opinion here, I'm okay with the current implementation.
| cd python-frontend/typeshed_serializer/resources/python-type-stubs | ||
| git sparse-checkout set --no-cone stubs/sklearn | ||
| git checkout |
There was a problem hiding this comment.
Do I understand correctly that what will actually happen here is that we first clone the whole microsoft/python-type-stubs repository then we basically remove all the unneeded parts through the sparse-checkout?
One thing I don't understand is why are we using no-cone here?
Also, could it be easier (from a sonar-python developer perspective) for us to fork this repository, commit the result of this operation and then add the fork as a submodule? It would be a bit more tedious to update, but it would reduce the impact in local development setup/CI configuration.
Even if the answer is yes, we could move on with the current solution and update that to a fork later (I believe we will need REs to create the fork for us).
There was a problem hiding this comment.
Yes no-cone is not relevant here. I forgot to remove it after some tests. The fork would be a good idea, I will create a ticket for it
| save_location: str | ||
| output_folder: str | ||
|
|
||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
Is the logging needed as part of the scikit-learn stubs change?
I'm not against having it, but I think this (as well as the formatting change) would be nice to have in a separate commit to clearly separate the QoL improvements for the serializer from the functional changes.
Also, I think if we have logs, we should have assertions for those logs in our tests (it's actually a point I like to make when referring to our string formatting rules, people tend not to make assertions on logs and we did find a few interesting issues in some big projects due to that).
There was a problem hiding this comment.
No, you are right this should have been in a different commit. For the sake of time, I will merge this PR. But I will better organize the commit history in the following PRs
| return thirdPartySymbolsMypy; | ||
| } | ||
|
|
||
| Map<String, Symbol> thirdPartySymbolsMicrosoft = getSymbolsFromProtobufModule(moduleName, PROTOBUF_THIRD_PARTY_MICROSOFT); |
There was a problem hiding this comment.
In terms of priority, does it make sense to have Microsoft stubs more prioritized than regular Typeshed?
In terms of conflict, I guess it may make sense that they have priority, but do we expect any actual conflict since the libraries contained in both are different?
In terms of likelihood to find what we're searching, maybe it makes sense to search regular Typeshed first?
And maybe it would even make sense to consider merging the protobuf output of Microsoft into PROTOBUF_THIRD_PARTY itself?
Really not sure about this one.
There was a problem hiding this comment.
Yes I think we could merge it. It would be less code to maintain
| @pytest.fixture(scope="session") | ||
| def microsoft_stubs(): | ||
| build_result, _ = MicrosoftStubsSerializer().get_build_result() | ||
| assert len(build_result.errors) == 323 |
There was a problem hiding this comment.
Does it even make sense to have an assert of this if it's not relevant?
I guess some kind of assert on the build result content to ensure it contains what we want makes sense, but why this one in particular?
There was a problem hiding this comment.
Yes this was mainly to acknowledge that there are errors we do not have to fix. I would add some assertions that's a good point
| @@ -1,6 +1,14 @@ | |||
| { | |||
There was a problem hiding this comment.
When encountering issue differences on ruling, can you clarify whether it's TP/FP and what is your overall evaluation of the differences?
Given these are on sklearn itself, I would assume these are FPs due to some mismatch between the sklearn version analyzed and the version of the stubs we use?
I know we have SONARPY-1518 to eventually deal with this, but another reviewer may not know that.
|
No description provided.