Repository navigation
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
|
👋 Thank you for your draft pull request! Do you know that you can use |
774ae4a to
ab5ea5d
Compare
|
Was this discussed at Astropy Coordination Meeting 2025? Could you please provide more context? Introducing a new required dependency is a big deal and should be well justified. Thanks! |
|
This topic was discussed tangentially at the Astropy Coordination Meeting 2025, in the context of improving our typing infrastructure and ensuring that type annotations don’t interfere with downstream packages. While typing itself is benign, issues arose from how type annotations were not resolvable at runtime. In particular, Sphinx attempts to resolve annotations during doc build to generate cross-references, and it does so using runtime analysis. Thus is doesn't see This approach has several concrete benefits:
Achieving this requires that certain typing constructs be available at runtime. Although we’ve historically treated The |
|
FWIW I'd rather prioritize scipy if we must add a new required dependency (#12448).
This is solely my personal opinion and I am sure we have a lot of those across the spectrum. |
I wish that were entirely the case. We've turned off showing typing in our docs and so have some downstream packages and for these packages nothing can break. But for other downstream packages malformed type annotations, like any broken code, can still poison docs. Thus the ongoing effort (e.g. #18358) to prevent the physical possibility of malformed type annotations. This effort to make Sphinx no longer a problem is made easier with And "MyST is an extension of CommonMark Markdown, that includes a rich additional syntax for technical authoring, and can integrate with Docutils and Sphinx." — https://myst-parser.readthedocs.io/en/latest/intro.html
|
|
Still, doc build and mypyc aren't something end-user need to worry about, so why not optional dev dependency, why required? |
|
What about here? Line 123 in ec2d974 |
They don't need to worry about it, but it's there, like the |
|
What is it that you would want to use |
|
|
|
I don't see a compelling need to require |
|
Thanks for the detailed breakdown. I’d argue that the bar for adding this dependency should be almost non-existently small. We have |
|
I can agree that this wouldn't be controversial if we were type checking, but we are not type checking. |
|
I guess we should start type checking! An allow-list for cosmology would be good. There's plenty of utility files we could start with. |
|
If you are serious about type checking in CI then you should at the very least bring it up in a developer telecon. |
c246f33 to
1007143
Compare
|
Forgot about |
|
I feel like it's fine to require this package at runtime but only as a backport component to the standard lib and only on older versions of Python. This is the only strategy I feel confortable with to ensure the following conditions
When I see it installed in a fresh env built against the newest stable Python I regard it as a bug. |
|
I agree the 2nd point is a potential major issue and a python version check should enable maintainers to easily spot it and prevent it from happening. |
|
Quite a few of the features @nstarman has mentioned are in the standard library since Python 3.12, so we will have much less need for |
|
True, but would it be 0 by then ? Also, and just in case, I don't think this is a compelling argument in favor of dropping 3.11, and I'd still much prefer if we kept support for it for (at least) another year. |
1007143 to
a2c071e
Compare
a2c071e to
b0e9222
Compare
|
@neutrinoceros See |
Signed-off-by: nstarman <[email protected]>
|
@neutrinoceros in 8ed6a30 I think I captured the full set of draft/deferred/withdrawn PEPs. |
|
I'm aware the
|
Yes, but that's not what the section
I agree! Bug-fix Backports:
Performance Backports:
Edit: updated list in #18360 (comment) But where performance is critical we certainly can guard behind Try running
That's just cold-start. And since Python has an import cache it's basically a one-time cost for importing anywhere in astropy. So in a package with both |
|
Ok in hindsight, that looks more reasonable to me now that you've laid out 2 things I wasn't aware of. Critically:
|
I expect similarly. TypeForm (py3.15), in particular seems fundamental to supporting the array api. |
pllim
left a comment
There was a problem hiding this comment.
Adding a mandatory dependency is a big deal. Was this discussed in Coordination Meeting or astropy-dev or anywhere? Do we actually have buy-in from the community? Thanks for your patience.
In the general case I very much agree, but this exact package
for all these reasons, I don't think it should be treated as a big deal. |
|
"no big deal" is not a precedent we should set. Please discuss with community. We have time as this is still very early in the release cycle. Thanks!
|
|
Sure, I'm not brushing off the need for discussion, I'm just saying I feel confident about this one not meeting controversy. |
|
Notes on https://peps.python.org/pep-0747/ (py3.15, if the PEP is accepted): PEP 747 introduces In this pattern, This capability is a component to Astropy’s roadmap of supporting multiple array types. With Being able to typecheck backend-specific behavior is crucial for preventing catastrophic performance failures in large computations. Using |
|
Having read through the discussion, I'm OK with adding this as a dependency. This is mostly based on the small footprint and low impact described by @neutrinoceros. |
|
Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then. In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary. If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock. If you believe I commented on this pull request incorrectly, please report this here. |
|
I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks! If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here. |
|
@astropy/astropy-core-maintainers are we planning to drop Python 3.11 for Astropy 8.1 or 9? |
|
I support this initiative. I still think however it would be an easier sell if it was only a dependency on older python versions where we actually want to use it. |
It's made by the core Python devs and we've had this as an on-again-off-again dependency.
4.6 goes back to 2023.
Why we should do this
1. Bug fixes:
__orig_bases__is consistently populated.ReadOnlywheninclude_extras=false.is_typeddict(TypedDict)correctly returnFalse.2. Performance Backports:
3. Supports uniform typing across python versions
It back ports functionality across python versions, so that the same type annotations can be used regardless of python version. This prevents the need for any
if sys.versionchecks in your code just to be able to use the correct type annotation.becomes
Here's a full list
Python 3.12 features (and backports)
TypeAliasType: Backport oftyping.TypeAliasType(PEP 695).Buffer: Backport ofcollections.abc.Buffer.get_original_bases: Backport oftypes.get_original_bases().override: Backport oftyping.override()(PEP 698).TypeVarenhancements:default=(PEP 696) andinfer_variance=(PEP 695).TypeVarTupleenhancements:default=(PEP 696) support and implementation.ParamSpecenhancements:default=(PEP 696) support and implementation.Protocolruntime behaviour: Backports performance improvements for runtime-checkable protocols.runtime_checkable: Backports performance improvements fortyping.runtime_checkable().Unpack: Backports therepr()change introduced by PEP 692.SupportsIndex: Backports performance improvements fortyping.SupportsIndex.NamedTuple: Backports addition of the__orig_bases__attribute onNamedTupleclasses.TypedDict: Backports consistent__orig_bases__support and related behaviour.Python 3.13 features (and backports)
NoDefault: Backport oftyping.NoDefault.ReadOnly: Backport oftyping.ReadOnly(PEP 705).TypeIs: Backport oftyping.TypeIs(PEP 742).get_protocol_members: Backport oftyping.get_protocol_members().is_protocol: Backport oftyping.is_protocol().deprecated: Backport ofwarnings.deprecated()(PEP 702).TypeVar.has_default,TypeVarTuple.has_default,ParamSpec.has_default:Python 3.14 features (and backports)
Reader: Backport ofio.Reader.Writer: Backport ofio.Writer.type_repr: Backport ofannotationlib.type_repr()used with PEP 649-style formats.Python 3.15 features (and backports / pre-standardisation)
NoExtraItems: Sentinel used when theextra_itemsclass argument toTypedDictis not provided.TypedDictclosed/extra_items/__closed__/__extra_items__: Backports PEP 728 features (closedandextra_items) fromtyping.TypedDict.4.
typing_extensionsis officially supported byruffIt's
ruff'spyupgraderule UP035 so it automatically handles...Current Astropy with Python 3.10+:
Future Astropy with Python 3.12+:
How do we prevent use of deferred/rejected PEP imports?
Very easily!
The one small thing is that this list does need to be maintained.
Thankfully the rate of PEPs and changes to
typing/typing_extensionsis relatively low.