Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

First pass on dealing with public private modules: Re-export attributes of public private modules through __all__ #54

Merged
merged 22 commits into from
Oct 7, 2024

Conversation

pavyamsiri
Copy link
Contributor

@pavyamsiri pavyamsiri commented Oct 5, 2024

Problem

This should close #48. This is a first pass on adding proper type annotations to stubs which previously used __getattr__.

Basic method

I basically called __all__ on all of the listed modules and then auto-generated each file with the members exposed through __all__.

I then went through each file to clean the formatting and also import the required attributes.

Issues with generated files

This did not work perfectly for all of them however.

Some stubs exported attributes that originate from generated files or non-native python scripts. This includes files like interpolate/dfitpack.pyi which relies on scipy/interpolate/src/dfitpack.pyf, and various files in sparse like sparse/bsr.pyi which rely on sparse/_sparsetools.py which needs to be generated by _generate_sparsetools.py.

I have put TODOs for all generated files explaining that they need dummy functions marked with deprecated and also for the files with issues with generated files, TODOs explaining the problem.

Spurious exports

I think there are some spurious exports i.e. some modules re-export classes from other modules, which are probably not meant to be part of the module namespace. For example some modules would re-export standard library modules like numbers or variables meant for testing only like test. I have removed some of these but not all of them. These would probably be fixed when the dummy functions are added.

@pavyamsiri pavyamsiri force-pushed the public-private-submodules branch from e87c4b8 to 060db27 Compare October 6, 2024 08:32
@jorenham jorenham self-requested a review October 6, 2024 22:20
@jorenham
Copy link
Owner

jorenham commented Oct 6, 2024

Auto-generating these __all__ seems like the smart move; well done.

There are a couple of minor mypy build errors in the stubtest CI step:

error: not checking stubs due to mypy build errors:
scipy-stubs/stats/mvn.pyi:1:1: error: Need type annotation for "__all__" (hint: "__all__: list[<type>] = ...")  [var-annotated]
    __all__ = []
    ^~~~~~~
scipy-stubs/stats/mvn.pyi:1:1: note: See https://kotlinisland.github.io/basedmypy/_refs.html#code-var-annotated for more info
scipy-stubs/stats/biasedurn.pyi:1:1: error: Need type annotation for "__all__" (hint: "__all__: list[<type>] = ...")  [var-annotated]
    __all__ = []
    ^~~~~~~
scipy-stubs/sparse/sputils.pyi:1:1: error: Need type annotation for "__all__" (hint: "__all__: list[<type>] = ...")  [var-annotated]
    __all__ = []
    ^~~~~~~
scipy-stubs/sparse/spfuncs.pyi:1:1: error: Need type annotation for "__all__" (hint: "__all__: list[<type>] = ...")  [var-annotated]
    __all__ = []
    ^~~~~~~
scipy-stubs/sparse/sparsetools.pyi:1:1: error: Need type annotation for "__all__" (hint: "__all__: list[<type>] = ...")  [var-annotated]
    __all__ = []
    ^~~~~~~
scipy-stubs/io/matlab/streams.pyi:1:1: error: Need type annotation for "__all__" (hint: "__all__: list[<type>] = ...")  [var-annotated]
    __all__ = []
    ^~~~~~~
scipy-stubs/io/matlab/mio_utils.pyi:1:1: error: Need type annotation for "__all__" (hint: "__all__: list[<type>] = ...")  [var-annotated]
    __all__ = []
    ^~~~~~~
scipy-stubs/io/matlab/mio5_utils.pyi:1:1: error: Need type annotation for "__all__" (hint: "__all__: list[<type>] = ...")  [var-annotated]
    __all__ = []
    ^~~~~~~
scipy-stubs/io/matlab/mio4.pyi:1:1: error: Need type annotation for "__all__" (hint: "__all__: list[<type>] = ...")  [var-annotated]
    __all__ = []
    ^~~~~~~
scipy-stubs/io/matlab/byteordercodes.pyi:1:1: error: Need type annotation for "__all__" (hint: "__all__: list[<type>] = ...")  [var-annotated]
    __all__ = []
    ^~~~~~~
scipy-stubs/fftpack/pseudo_diffs.pyi:4:1: error: Module "scipy.fftpack" has no attribute "convolve"  [attr-defined]
    from . import convolve
    ^

The missing module is a cython one: https://github.com/scipy/scipy/blob/v1.14.1/scipy/fftpack/convolve.pyx

Once these are fixed, we'll be able to see whether if this will decrease the amount of stubtest errors.

And for what it's worth, pyright seems to like this change 😄

$ git checkout master --quiet && poe pyright | tail -n 1
5399 errors, 0 warnings, 0 notes
$ git checkout pr/pavyamsiri/54 --quiet && poe pyright | tail -n 1
5293 errors, 0 warnings, 0 notes

@jorenham
Copy link
Owner

jorenham commented Oct 6, 2024

And in the scipy.interpolate.dfitpack case, I see no problem in using the dummiest of functions for them, as they're deprecated anyway. So something like

@deprecated("will be removed in SciPy v2.0.0")
def parcur(*args: object, **kwds: object) -> object: ...

should be perfectly fine

@jorenham
Copy link
Owner

jorenham commented Oct 6, 2024

Were you planning on solving those TODOs in this PR? Because otherwise it might help to open a separate issue for it

@pavyamsiri
Copy link
Contributor Author

Were you planning on solving those TODOs in this PR? Because otherwise it might help to open a separate issue for it

I didn't mean to close the issue I linked. This is just a partial implementation of the desired implementation that being adding dummy functions for all the files I listed in the issue. This however would require proper type annotations for each of the functions that get re-exported which essentially would require all the affected modules to be complete so I thought we should at least have some progress.

Unless you want to have the dummy functions not need type annotations? I could probably spend some time writing out the function stubs in that case and get all the TODOs completed.

@jorenham
Copy link
Owner

jorenham commented Oct 7, 2024

Unless you want to have the dummy functions not need type annotations? I could probably spend some time writing out the function stubs in that case and get all the TODOs completed.

Your call; I'd be happy to merge this without the additional stubs, once the stubtest build errors are resolved.

This is to be able to export out `convolve` without needing the Cython
module `convolve.pyx` to have type stubs.
@pavyamsiri
Copy link
Contributor Author

Unless you want to have the dummy functions not need type annotations? I could probably spend some time writing out the function stubs in that case and get all the TODOs completed.

Your call; I'd be happy to merge this without the additional stubs, once the stubtest build errors are resolved.

Okay I can fix the build errors. I think I want to leave the dummy functions for a later PR.

@pavyamsiri
Copy link
Contributor Author

pavyamsiri commented Oct 7, 2024

And in the scipy.interpolate.dfitpack case, I see no problem in using the dummiest of functions for them, as they're deprecated anyway. So something like

@deprecated("will be removed in SciPy v2.0.0")
def parcur(*args: object, **kwds: object) -> object: ...

should be perfectly fine

This level of type annotations is fine for all of the functions and classes affected by the v2 deprecation right?

@jorenham
Copy link
Owner

jorenham commented Oct 7, 2024

And in the scipy.interpolate.dfitpack case, I see no problem in using the dummiest of functions for them, as they're deprecated anyway. So something like

@deprecated("will be removed in SciPy v2.0.0")
def parcur(*args: object, **kwds: object) -> object: ...

should be perfectly fine

This level of type annotations is fine for all of the functions and classes affected by the v2 deprecation right?

These functions shouldn't be used anymore, so I see no reason why we should put effort into making them easier to use 🤷🏻

@jorenham
Copy link
Owner

jorenham commented Oct 7, 2024

Stubtest can run again; good!

But there are some new stubtest errors, most of them similar to

scipy.io.arff.tokenize_attribute is not present at runtime

I'm guessing it has to do with the import * statements, because it looks like stubtest looks at all available names in a module, not just those explicitly exported in __all__ 🤔

@pavyamsiri
Copy link
Contributor Author

I think I will just add the dummy functions at this point because the import issues will be fixed as well. Might take a while though.

As the automated CI hasn't been added yet, what are the general things you do to test the stubs?

I didn't run them locally because my patch wasn't supposed to be entirely correct due to the lack of dummy functions and because it spanned many modules I didn't think I could parse through the errors.

@jorenham
Copy link
Owner

jorenham commented Oct 7, 2024

In this case I just compared the stubtest output from the CI with that of the latest master branch run. But locally you could do a poe stubtest to get the same output

The linter does not like operations on `__all__` and before this patch,
`_arffread` had no explicit `__all__` which lead to stubtest errors.
Additionally add a stub file for the `convolve` module which was missing
one as it was originally a Cython file. This module is not fully typed.
@pavyamsiri pavyamsiri force-pushed the public-private-submodules branch from daf7419 to 1757d19 Compare October 7, 2024 14:34
@pavyamsiri
Copy link
Contributor Author

@jorenham I think all the dummy functions are in place and there should be less or the same amount of errors as the master branch.

I was not very DRY with the dummy functions because importing in dummy definitions would be complicated so there are repeats of stubs. Hopefully we never have to touch this stuff again though.

@jorenham
Copy link
Owner

jorenham commented Oct 7, 2024

I was not very DRY with the dummy functions because importing in dummy definitions would be complicated so there are repeats of stubs. Hopefully we never have to touch this stuff again though.

Unfortunately DRY isn't really feasible in practice when writing stubs I'm afraid... And that's precisely the reason why I'm building unpy BTW.

@jorenham
Copy link
Owner

jorenham commented Oct 7, 2024

Wow; the amount of stubtest error went from 1813 (on master) to 1201 with this PR, so you solved 34% of the problems right here 🎉!

@jorenham jorenham merged commit ce78eb0 into jorenham:master Oct 7, 2024
1 check passed
@jorenham
Copy link
Owner

jorenham commented Oct 7, 2024

Thanks @pavyamsiri !

@pavyamsiri pavyamsiri deleted the public-private-submodules branch November 3, 2024 23:44
@jorenham jorenham added stubs: improvement Improve or refactor existing annotations topic: deprecation Dealing with deprecated features scipy.* labels Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scipy.* stubs: improvement Improve or refactor existing annotations topic: deprecation Dealing with deprecated features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

What to do with deprecated public submodules
2 participants