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

Use pre releases to allow community find bug or patch its packages. #185

Closed
Czaki opened this issue May 23, 2023 · 19 comments · Fixed by #197
Closed

Use pre releases to allow community find bug or patch its packages. #185

Czaki opened this issue May 23, 2023 · 19 comments · Fixed by #197

Comments

@Czaki
Copy link

Czaki commented May 23, 2023

The typing-extension package is widely used in many projects. It happens that sometimes some package does to strong assumptions (pydantic/pydantic#5821).

It would be nice if typing_extension could create a pre (rc) release and give a few days (for example week) for the community to test it. (example of a project that partially uses this approach is, for example, numpy https://pypi.org/project/numpy/#history)

@hauntsaninja
Copy link
Collaborator

Thanks for the suggestion!

I definitely agree that typing_extensions is widely used enough that we should try to take steps to ensure people don't break (especially since widespread pinning of typing_extensions can really hurt the adoption of newer type system features)

That said, I'm not sure how much people actually test pre-releases, so I'm a little wary of how much this would actually help. Maybe someone from pydantic can chime in on whether they'd commit to testing prereleases?

We have some other levers here, e.g. we could consider a policy of yanking releases that cause some amount of breakage and waiting a set amount of time before re-releasing. For the specific pydantic issue, it could be worth documenting Jelle's suggestion here: pydantic/pydantic#5821 (comment) / completing #50 could make this mildly easier.

@JelleZijlstra
Copy link
Member

Another approach could be to run the test suite of some projects that rely heavily on runtime introspection (e.g., pydantic, typing-inspect, pyanalyze) in our CI.

@mapadofu
Copy link

I assume this is in relation to the way that 4.6.0 breaks some other packages (I saw it with geojson-pydantic/pydantic).

Is the behavior of 4.6.0 the desired behaviour that these other packages need to conform to or is the plan to regain backward compatability?

@JelleZijlstra
Copy link
Member

The 4.6.0 changes are desired behavior. (Indeed, Pydantic apparently asked for them in the first place.)

With runtime introspection of types, almost any change can break some user. I think we'll put some suggestions for writing robust type-introspection code in the docs..

@Czaki
Copy link
Author

Czaki commented May 24, 2023

I dont know how many projects has pre-release test suite. In napari we have it and it happens to use to report problem to upstream packages.

Adoption of pre-release by typing extension may help to propagate pre-release CI jobs in multiple projects.

@grochmal
Copy link

Looking at the pydantic issue (and also a typed-argument-parse issue i did run into and opened on their github) i believe that the source of trouble is the change to the if at: https://github.com/python/typing_extensions/blob/main/src/typing_extensions.py#L278
between versions 4.5.0 and 4.6.0

In 4.5.0 we got:

# 3.8+:
if hasattr(typing, 'Literal'):
    Literal = typing.Literal
# 3.7:
else:
    class _LiteralForm(typing._SpecialForm, _root=True):
...

So python 3.8.x and 3.9.x went through the first branch, while <=3.7.x fell through to the else

In 4.6.0

# A Literal bug was fixed in 3.11.0, 3.10.1 and 3.9.8
if sys.version_info >= (3, 10, 1):
    Literal = typing.Literal
else:
    # ... many, many things happen here

The branching is now different for python >=3.8.0 <=3.10.0 . The intention is to hide a few bugs in python 3.9 and 3.10 - probably a good idea. But the side effect is that now code tested in the wild on python <3.8 is now applied to installations with python 3.8 and 3.9 - and chaos ensures.

Could we instead make the if as if sys.version_info >= (3, 8, 0) ?

Not very clean on python bugs, but ensures backward compatibility with typing-extensions==4.5.0

@adriangb
Copy link
Contributor

adriangb commented May 24, 2023

I'd like to say that Pydantic does a lot of gnarly stuff internally and this issue was mainly caused by us not handling things well enough and is not really typing-extensions responsibility. We should be doing something like what Jelle suggested (which Shantanu also pointed out above).

Pydantic depends heavily enough on typing-extensions that we should probably be testing against main in our CI, which achieves most of what pre-releases do without the burden on the maintainers here.

@Czaki
Copy link
Author

Czaki commented May 24, 2023

@adriangb It will not help to find this issue. The main branch is pydantic 2. As pydantic 2 is incompatible with 1. Then for long time you should also do this on 1.10.X-fixes branch.

Also, I do not meet projects that run regular tests against the latest release. All I observe are tests for the current development branch.
So if there is a need for a bugfix release it is often raised by maintainers of dependent projects.

@adriangb
Copy link
Contributor

adriangb commented May 24, 2023

Then for long time you should also do this on 1.10.X-fixes branch.

I'm not saying we will, but I agree with you that we should.

I do not meet projects that run regular tests against the latest release

I agree I haven't seen this, but it also often happens that the bug is still present in HEAD unless it got accidentally patched. But the pre-releases also don't solve this, and that's beyond the scope of this issue.

@Czaki
Copy link
Author

Czaki commented May 24, 2023

I agree I haven't seen this, but it also often happens that the bug is still present in HEAD unless it got accidentally patched. But the pre-releases also don't solve this, and that's beyond the scope of this issue.

With pre-release there is a big probability that you will get bug report, for example, from napari.

@grochmal
Copy link

pydantic is gnarly alright, yet other libraries also depend on similar hacky code. Mostly due to how the Literal type was evolving during python 3.8.

I have reported an issue on typed-argument-parser which also uses some hacky code to identify the Literal type. The typed-argument-parser issue with typing-extensions==4.6.0 is quite similar to the pydantic but not exactly the same issue.

@JelleZijlstra
Copy link
Member

I opened #189 with some advice for users who rely on runtime introspection of types.

@AlexWaygood
Copy link
Member

AlexWaygood commented May 24, 2023

Yes, it is true that pydantic wasn't the only package broken unexpectedly broken here. As well as pydantic and typed-argument-parser, typing_inspect also made the same incorrect assumption: ilevkivskyi/typing_inspect#100. So, while this might not have been our "fault", I agree that it's in everybody's benefit if we try to figure out how to make this situation less likely in the future.

I'm not sure pre-releases would be very helpful; as @hauntsaninja says, I'm not convinced that very many people test with pre-releases. I like @JelleZijlstra's idea of running the tests for pydantic, and maybe some other projects as well, over at typing_extensions. (We wouldn't necessarily have to do it on every PR or push -- maybe we could have a nightly cron job?)

In terms of making it easier for other projects to follow "introspection best practices": the docs @JelleZijlstra proposes in #189 look great. I also wonder if we could maybe provide an introspection helper like @JelleZijlstra's suggestion here ourselves, as part of the typing_extensions API? It seems like lots of projects have a need to do introspection of typing constructs at runtime, and it's quite easy to get it wrong.

@rsokl
Copy link

rsokl commented May 24, 2023

FWIW hydra-zen's CI runs against release candidates and would have caught #181 . It also would have been able to warn beartype that its support for typing_extensions.Protocol was going to break with the release of 4.6.0 .

@ilevkivskyi
Copy link
Member

@AlexWaygood if you will at some point add some daily tests for dependent packages, I would like to opt-in typing_inspect, it relies heavily on various typing_extensions internals (running against rc/dev will not help much since there are only a handful commits per year).

@Czaki
Copy link
Author

Czaki commented May 24, 2023

@ilevkivskyi In napari we have cron job for pre release tests. Maybe it could be also a good solution for you?

@ilevkivskyi
Copy link
Member

Hm, yeah, this is an interesting idea.

@AlexWaygood
Copy link
Member

AlexWaygood commented May 25, 2023

@AlexWaygood if you will at some point add some daily tests for dependent packages, I would like to opt-in typing_inspect

(That sounds good to me, but I'm not actually a typing_extensions maintainer right now, just a triager and regular contributor, so it's not really my call :)

@JelleZijlstra
Copy link
Member

I think doing release candidates is a good idea. I opened #197 to formalize such a policy.

I also opened issue #198 to track the idea floated above of running third-party projects' test suites against typing-extensions regularly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants