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

What to do with deprecated public submodules #48

Closed
pavyamsiri opened this issue Oct 2, 2024 · 11 comments · Fixed by #54 or #79
Closed

What to do with deprecated public submodules #48

pavyamsiri opened this issue Oct 2, 2024 · 11 comments · Fixed by #54 or #79
Labels
topic: deprecation Dealing with deprecated features

Comments

@pavyamsiri
Copy link
Contributor

pavyamsiri commented Oct 2, 2024

There are various currently public submodules in scipy that are to be deprecated in scipy v2.0.0 such as io.matlab.mio. These files simply have two functions that are really overrides of the standard module functions __dir__ and __getattr__ in order to display deprecation warnings.

For example this is scipy/io/matlab/mio.py.

# This file is not meant for public use and will be removed in SciPy v2.0.0.
# Use the `scipy.io.matlab` namespace for importing the functions
# included below.

from scipy._lib.deprecation import _sub_module_deprecation

__all__ = ["loadmat", "savemat", "whosmat"]  # noqa: F822

def __dir__():
    return __all__


def __getattr__(name):
    return _sub_module_deprecation(sub_package="io.matlab", module="mio",
                                   private_modules=["_mio"], all=__all__,
                                   attribute=name)

The most obvious type hints for this def __dir__() -> list[str]: ... and def __getattr__(name: str) -> Any. Is this good enough?

Would there be any alternatives to using Any in the case of the second? I feel like this would obscure the type of attribute being accessed but I'm not sure giving concrete types are possible at all or desirable.

If the type hints I suggested are okay I can go and make a PR to add these type annotations to every instance of this pattern.

@pavyamsiri
Copy link
Contributor Author

Actually I saw how @jorenham has done it in files like scipy/integrate/vode.pyi.

# This file is not meant for public use and will be removed in SciPy v2.0.0.
from typing_extensions import Never, deprecated

__all__: list[Never] = []

@deprecated("will be removed in SciPy 2.0.0.")
def __dir__() -> list[Never]: ...
@deprecated("will be removed in SciPy 2.0.0.")
def __getattr__(name: str) -> Never: ...  # pyright: ignore[reportIncompleteStub]

Just need to copy it like this? This should be simple to auto-replace using a script I think.

@jorenham
Copy link
Owner

jorenham commented Oct 2, 2024

Yea there's quite a couple of those; good question

I'm not sure anymore whether __getattr__ and __dir__ are the way to go actually.
Yesterday, I even went as far as to prohibit their use in jorenham/unpy#79

In other modules, e.g. special, I left those out, see e.g. https://github.com/jorenham/scipy-stubs/blob/master/scipy-stubs/special/sf_error.pyi

Usually, these deprecated modules are simply re-exports of other (private) modules, so you could just place a comment saying that it's deprecated, and then just re-export them.

I also wouldn't mind if you'd use a "dummy" function with a @deprecated, e.g.

@deprecated("don't")
def loadmat(*args: object, **kwds: object) -> object: ...

Either way, we should probably try to minimize the amount of stubtest errors. In cases like these I'm fine with ignoring an error or two in the allowlist, but that won't even be necessary when you go for the re-export or dummy-function approach.

@jorenham
Copy link
Owner

jorenham commented Oct 2, 2024

it's too bad that warnings.deprecated doesn't work on entire modules 🤷🏻

@pavyamsiri
Copy link
Contributor Author

pavyamsiri commented Oct 2, 2024

Yes in regards to "I'm not sure anymore whether getattr and dir are the way to go actually.", it seems to obscure the type checker's ability to reason about the attributes' type.

Does adding @deprecated warn the user via the LSP that it is deprecated? That might make it better than just simply re-exporting because the comment can't be read unless you jump to the type definition.

This would be a lot more work though because you would need to duplicate the type annotations rather than just the names like in the re-export approach.

@jorenham
Copy link
Owner

jorenham commented Oct 2, 2024

Yes in regards to "I'm not sure anymore whether getattr and dir are the way to go actually.", it seems to obscure the type checker's ability to reason about the attributes' type.

You're right; and that's precisely the reason why I decided to disallow them in unpy: __getattr__ and __dir__ are dynamic, stubs are static.

Does adding @deprecated warn the user via the LSP that it is deprecated? That might make it better than just simply re-exporting because the comment can't be read unless you jump to the type definition.

I know that at least VScode supports it in a very helpful manner. I don't think that mypy currently supports it, but pyright (==pylance) do.

This would be a lot more work though because you would need to duplicate the type annotations rather than just the names like in the re-export approach.

Yea I agree that it's not very DRY. But unfortunately, code duplication is something very common in python typing (which I'm currently trying to tackle through unpy).

But I consider @deprecated as a "nice to have", so adding it in a later iteration would be fine too.
Perhaps this is also something that we can automate with unpy.

The main priority is to have complete scipy-stubs so that it's complete, and valid (at least according to stubtest, (based)mypy, (based)pyright, and ruff), and most importantly, helpful.

Either way, I guess I'm fine with whatever you decide on this point.
If stubtest, ruff and basedpyright are happy with it, then I am too.

@pavyamsiri
Copy link
Contributor Author

Ok got it.

I am actually not too familiar with the tools as of right now. What is the expected output of stubtest, ruff and basedpyright as of now, considering that the stubs are incomplete? I think I tried running stubtest but it just complained about missing stubs.

@jorenham
Copy link
Owner

jorenham commented Oct 2, 2024

Assuming that you have poetry and poe installed, then you can run stubtest on a specific directory as e.g.

$ poe stubtest scipy.special -- --ignore-unused-allowlist
Poe => stubtest --mypy-config-file=pyproject.toml --allowlist=tests/stubtest/allowlist.txt scipy.special --ignore-unused-allowlist
Success: no issues found in 100 modules

similarly, you can run

$ poe mypy special
Poe => mypy --config-file=pyproject.toml scipy-stubs/special
Success: no issues found in 19 source files
$ poe pyright special
Poe => basedpyright scipy-stubs/special
0 errors, 0 warnings, 0 notes

and

$ poe lint
Poe => ruff check
All checks passed!

(so ruff is already happy with everything)

The "unfinished" modules indeed produce errors in stubtest at the moment. So when you're tackling a package or module, you can consider it "complete" if there are no stubtest errors left.


I'm planing to write a contributing guide soon, see #50

@jorenham
Copy link
Owner

jorenham commented Oct 8, 2024

This apparently was auto-closed, sorry for that.

@pavyamsiri
Copy link
Contributor Author

I think my PR mostly closes this issue. There might be a few more deprecated modules though because I only did ones where the stub had an exact match to the template I made so I guess until they are done we can leave this issue open?

@jorenham
Copy link
Owner

jorenham commented Oct 8, 2024

so I guess until they are done we can leave this issue open?

Yea sounds like a plan

@jorenham
Copy link
Owner

resolved through #54 and #79 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: deprecation Dealing with deprecated features
Projects
None yet
2 participants