DEP: adjust requirements for extra dependency on typing-extensions by neutrinoceros · Pull Request #15686 · astropy/astropy

neutrinoceros · 2023-12-06T08:31:53Z

Description

@hamogu @eerovaher

While reviewing #15685, I noticed that requirements on typing_extensions needed slight adjustements:

we use it only for the Self symbol, which was added in version 4.0.0
this symbol is in the standard library (typing.Self) in Python 3.11 and newer, so I made this requirement py310- only

I also found a note in a doctoring that doesn't seem relevant now that support for Python 3.8 was dropped.

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2023-12-06T08:32:28Z

neutrinoceros · 2023-12-06T11:00:56Z

I guess that the adjustment in minimal required version is technically a bug fix, so I'd suggest to backport this.

eerovaher · 2023-12-06T14:16:53Z

 if TYPE_CHECKING:
+    import sys
    from typing import Any, Callable

-    from typing_extensions import Self
-
    from astropy.units import UnitBase

+    if sys.version_info >= (3, 11):
+        from typing import Self
+    else:
+        from typing_extensions import Self


This if TYPE_CHECKING: block was added in #15354. An early version of that pull request did include explicitly version dependent code, but during the review @nstarman pointed out that Ruff (specifically UP035) knows enough about typing and typing_extensions that it can automatically change the import when support for Python 3.10 is dropped. The change proposed here is not necessary.

Ruff (specifically UP035) knows enough about typing and typing_extensions that it can automatically change the import when support for Python 3.10 is dropped.

good to know, but that's not why I added it here; my reasoning is that, I can't make typing-extensions a py310- only extra dependency without handling the py311+ case explicitly in code.
If you guys still prefer to keep the current style that's fine with me, and in that case, please feel free to close !

Then it might be a good idea to have explicitly version-dependent code here after all. Note that Ruff rule UP036 recognizes obsolete sys.version_info comparisons, so there's no reason we should use astropy.utils.compat.PYTHON_LT_3_11 here.

I still think it's best to not version check on typing_extension imports. The package does it internally and its MO is to make using typing features seamless. Later version-dependent code doesn't care where Self is from so long as it's there. And with typing_extensions it always will be.

#minimum-effort.

Is this conversation resolved or not? Is this PR good to merge? @eerovaher ?

eerovaher · 2023-12-06T14:22:44Z

        -----
-        With Python 3.9+ or :mod:`typing_extensions`, |Quantity| types are also
-        static-type compatible.
+        |Quantity| types are also static-type compatible.


In the pull request @nstarman opened to drop support for Python 3.9 he also found some code and documentation that should or could have been updated when support for Python 3.8 was dropped. That pull request has not been merged yet. Perhaps it would be a good idea to open a separate pull request for removing the obsolete mentions of Python 3.9 like this.

You must be referring to #15603. I notice this specific line isn't touched there. I'm happy to open another PR with just this (and possibly other similar changes if I find any), but I'm also fine if @nstarman wants to add it to his PR instead !

The problem with #15603 is that it seems to be stuck because the reference images for image comparison tests need to be regenerated. It would be better to split off things from there in new pull requests instead of adding more things to it that would then get stuck too.

Agreed. Anything that can be paired off is good. Feel free to 🍒pick commits.

Ok, I'll move it to a separate branch, and will open a PR in a few days with maybe more of the same !

eerovaher · 2023-12-06T14:24:59Z

    "fsspec[http]>=2023.4.0",
    "s3fs>=2023.4.0",
    "pre-commit",
+    "typing_extensions>=4.0.0 ; python_version < '3.11'",


It is good to try to keep track of why typing_extensions is needed, but it is also important to record the reason in commit messages.

nstarman

Thanks @neutrinoceros! Looks good as is, but I have a small suggestion. Also @eerovaher's comment on comments is good, lest we forget why typing_extensions is version-pinned

eerovaher · 2023-12-06T17:25:21Z

Do we actually need typing_extensions as a direct dependency anyways? It should only ever be used in if TYPE_CHECKING: blocks, which are not executed at runtime.

neutrinoceros · 2023-12-06T17:32:17Z

Do we actually need typing_extensions as a direct dependency anyways?

it's currently only included as an extra dependency (part of the all target), which seems reasonable ?

neutrinoceros · 2023-12-06T17:38:34Z

@eerovaher @nstarman I just removed the adjacent-but-not-strictly-related part of the patch and amended the commit message with more details.

eerovaher · 2023-12-06T17:39:13Z

If we don't list typing_extensions as a dependency at all then we don't have to worry about which Python versions it should be installed with, and then it's clear that we don't need any version dependent code inside if TYPE_CHECKING: blocks either.

neutrinoceros · 2023-12-06T17:50:04Z

Then I'll leave it to @nstarman to decide what to do here since he previously approved, but again, no harm done in closing this PR if we don't collectively think it's needed :)

eerovaher · 2023-12-06T18:02:11Z

I'm not suggesting to close this pull request without merging, I'm suggesting remove typing_extensions from the dependency list.

nstarman · 2023-12-06T21:05:27Z

I think if someone installs Astropy[all] they should get everything, including static type time stuff. Perhaps we should add an install option [type] that is included in all. Because with Protocols and API we can support interfaces without run-time dependencies. So IMO let's leave it all for now and follow up with a type install option

neutrinoceros · 2023-12-06T21:50:12Z

Fine by me @nstarman. I'm also happy to open another PR to implement your proposed [type] extra, if you'd like.

nstarman · 2023-12-06T21:51:22Z

Fine by me @nstarman. I'm also happy to open another PR to implement your proposed [type] extra, if you'd like.

Much appreciated!

neutrinoceros · 2023-12-07T10:42:13Z

done in #15689

nstarman · 2023-12-07T18:56:23Z

Merge this first or what's the order?

neutrinoceros · 2023-12-07T21:26:19Z

@nstarman any order is fine, I'll just need to rebase whatever comes in second

eerovaher · 2023-12-07T21:36:45Z

What is this pull request supposed to do when #15689 is merged?

nstarman · 2023-12-07T22:10:02Z

Add the info. 2 small PRs. Could combine, but it's fine as is, reflecting our discussions.
@eerovaher, is this good to merge? I still think we can drop the version check in modeling, but that's entirely stylistic since typing_extensions does it internally and we have ruff's pyupgrade to take care of all future cleanup.

neutrinoceros · 2023-12-08T07:45:55Z

rebased and updated. Now it should be clear what it does on top of #15689 , and it's good to go !

eerovaher · 2023-12-08T12:35:34Z

I don't think the version check is necessary for preventing unwanted installations of typing_extensions now that #15689 is merged. If users install astropy[typing] they probably want typing_extensions. If they install astropy[all] they shouldn't be surprised if they end up with packages they are not actually using.

neutrinoceros · 2023-12-08T12:51:35Z

If users install astropy[typing] they probably want typing_extensions.

I respectfully disagree here, but I won't insist. Again, I'm fine with this PR being closed if it's controversial.

neutrinoceros · 2023-12-08T13:12:39Z

I believe the only remaining failure is due to #15692, which I think can be resolved separately, so I'll start working on that now.

pllim · 2024-01-06T04:41:09Z

Will this fix #15819 ? If so, can we wrap this up for merge? Thanks!

pllim · 2024-01-06T04:41:30Z

Actually needs a rebase, and then I can slap "Extra CI" on this, to be sure.

neutrinoceros · 2024-01-06T07:58:15Z

Will this fix #15819 ?

I don't think so, but I'll rebase now anyway !

At the moment, typing-extensions is only ever used to replace `typing.Self`, which is included in the standard library from Python 3.11 on. The backport symbol, `typing_extensions.Self`, requires version 4.0 (or newer).

pllim · 2024-01-06T19:22:32Z

Hmm link check is green here. 🤷

nstarman · 2024-01-07T16:37:08Z

My personal preference would be to just have typing_extensions as a standard dependency of Astropy and let ruff switch imports from typing_extensions to typing per our minimum Python version. typing_extensions is developed within the core Python org (https://github.com/python/typing_extensions), so this is a very safe approach. That said, since we have not yet committed to having typing_extensions as a dependency, this PR is good to merge. Thanks @neutrinoceros!

neutrinoceros requested a review from a team as a code owner December 6, 2023 08:31

github-actions Bot added units modeling installation labels Dec 6, 2023

neutrinoceros mentioned this pull request Dec 6, 2023

Start enforcing Ruff rule ANN201 #15685

Merged

2 tasks

pllim requested review from eerovaher and nstarman December 6, 2023 13:40

pllim added this to the v6.1.0 milestone Dec 6, 2023

pllim added the no-changelog-entry-needed label Dec 6, 2023

eerovaher reviewed Dec 6, 2023

View reviewed changes

nstarman approved these changes Dec 6, 2023

View reviewed changes

neutrinoceros marked this pull request as draft December 6, 2023 17:33

neutrinoceros force-pushed the typing_extensions_req branch from 0cd8d5c to 2d1e924 Compare December 6, 2023 17:37

neutrinoceros marked this pull request as ready for review December 6, 2023 17:38

neutrinoceros mentioned this pull request Dec 7, 2023

DEP: add a [typing] extra dependency target for type-check-only dependencies #15689

Merged

1 task

neutrinoceros force-pushed the typing_extensions_req branch from 2d1e924 to d703f40 Compare December 8, 2023 07:45

DEP: adjust requirements for extra dependency on typing-extensions

169f461

At the moment, typing-extensions is only ever used to replace `typing.Self`, which is included in the standard library from Python 3.11 on. The backport symbol, `typing_extensions.Self`, requires version 4.0 (or newer).

neutrinoceros force-pushed the typing_extensions_req branch from d703f40 to 169f461 Compare January 6, 2024 07:59

pllim added the Extra CI Run cron CI as part of PR label Jan 6, 2024

pllim merged commit e955c72 into astropy:main Jan 7, 2024

neutrinoceros deleted the typing_extensions_req branch January 7, 2024 16:42

This was referenced Jan 11, 2024

Cosmo: type annotate the optimization functions #15853

Merged

Create units.typing module with QuantityLike #15860

Merged

This was referenced Jan 11, 2024

change convention for typing_extensions #15867

Closed

change convention for typing_extensions #15870

Merged

Uh oh!

Conversation

neutrinoceros commented Dec 6, 2023

Description

Uh oh!

github-actions Bot commented Dec 6, 2023

Uh oh!

neutrinoceros commented Dec 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nstarman Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nstarman left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerovaher commented Dec 6, 2023

Uh oh!

neutrinoceros commented Dec 6, 2023

Uh oh!

neutrinoceros commented Dec 6, 2023

Uh oh!

eerovaher commented Dec 6, 2023

Uh oh!

neutrinoceros commented Dec 6, 2023

Uh oh!

eerovaher commented Dec 6, 2023

Uh oh!

nstarman commented Dec 6, 2023

Uh oh!

neutrinoceros commented Dec 6, 2023

Uh oh!

nstarman commented Dec 6, 2023

Uh oh!

neutrinoceros commented Dec 7, 2023

Uh oh!

nstarman commented Dec 7, 2023

Uh oh!

neutrinoceros commented Dec 7, 2023

Uh oh!

eerovaher commented Dec 7, 2023

Uh oh!

nstarman commented Dec 7, 2023

Uh oh!

neutrinoceros commented Dec 8, 2023

Uh oh!

eerovaher commented Dec 8, 2023

Uh oh!

neutrinoceros commented Dec 8, 2023

Uh oh!

neutrinoceros commented Dec 8, 2023

Uh oh!

pllim commented Jan 6, 2024

Uh oh!

pllim commented Jan 6, 2024

Uh oh!

neutrinoceros commented Jan 6, 2024

nstarman Dec 6, 2023 •

edited

Loading

nstarman left a comment •

edited

Loading