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

Rip out TaskLocal objects and fix system task context inheritance. #499

Closed
wants to merge 1 commit into from
Closed

Rip out TaskLocal objects and fix system task context inheritance. #499

wants to merge 1 commit into from

Conversation

Fuyukai
Copy link
Member

@Fuyukai Fuyukai commented Apr 14, 2018

Closes #289, and removes all the old TaskLocal code deprecated in the last version.

@smurfix
Copy link
Contributor

smurfix commented Apr 14, 2018

Umm …

  • we should delay ripping out TaskLocal until after the deprecation time has passed. I want to impose some rules for minimum time until a deprecated feature is removed, but I don't want to have to delay a release just because that would otherwise be violated. We didn't decide on a timeframe yet but IMHO a month or two after the release in which the deprecation notice appeared and an intervening release is appropriate.
  • this patch has at least two other ideas in it, and at least one of them causes massive failure. I don't have a problem with test failues, that's why we test, but could you please limit yourself to one feature per commit? ;-)

@Fuyukai
Copy link
Member Author

Fuyukai commented Apr 14, 2018

Hm, alright. It's just that ripping it out and then updating system tasks at the same time is the (imo) logical process, but otherwise I'll close this for now.

(also, I was going to fix the massive failures but I got sidetracked doing something else)

@Fuyukai Fuyukai closed this Apr 14, 2018
@njsmith
Copy link
Member

njsmith commented Apr 15, 2018

It's true it would be easier to review as two separate changes. I think they should be mostly orthogonal anyway?

OTOH the big advantage of the "one release of deprecation" model is indeed that it lets you immediately rip out the old stuff. Like discussed in #1 (comment), there is a real cost to more complex deprecation processes, and if that's helping our users then that's fine, but we should also be careful not to be making unnecessary work for ourselves. We have no shortage of necessary work :-).

I get why it would make sense to have a minimum waiting period between the release deprecating something and the release removing it. I don't understand why it should also depend on how many releases happen in between – if something is deprecated for, say, a month, then anyone who upgrades during that period will see the warnings, and anyone doesn't, won't. So why does it matter whether a second release happened in week two or not? But @Fuyukai I know you suggested it in the first place and now @smurfix, you're keen on it too, so maybe I'm missing something.

@smurfix
Copy link
Contributor

smurfix commented Apr 15, 2018

"Deprecate for a month" doesn't work if there was no release within that month, because those of our users who get the code from PyPI won't see it – and there's going to be more of these as Trio gains traction.
"Deprecate, release, wait one month, then rip out the code" would work for me. I don't think we need to go the PendingDeprecation route Python itself uses … at least not yet. ;-)

@jab
Copy link
Member

jab commented Apr 15, 2018

Quoting https://semver.org/#spec-item-4:

  1. Major version zero (0.y.z) is for initial development. Anything may change at any time. The public API should not be considered stable.

Based on this, as long as Trio is pre-1.0, I think the Trio maintainers should do whatever is the least work to move the project forward. So e.g. intervening releases, deprecation warnings, mandatory waiting periods, etc. are all more than users should expect. And for whatever it's worth, this particular user would rather see maintainers have less work at this point in the project so that it can realize its ultimate potential all the sooner. As a user with lots of experience using pre-1.0 software releases, it's always seemed like good practice anyway to peg to exact versions of all pre-1.0 releases I depend on, for exactly the reasons spelled out in the semver spec.

@smurfix
Copy link
Contributor

smurfix commented Apr 15, 2018

Well, finding and removing deprecated code isn't any more or less work whether you do it sooner or later. So yes, semver says 0.x.y versions may change basically whenever, but that doesn't prevent us from adopting slightly stronger stability guidelines.

@njsmith
Copy link
Member

njsmith commented Apr 16, 2018

"Deprecate for a month" doesn't work if there was no release within that month

Right, if we have a "deprecate for X time" policy, then the timer should start counting from the first release that includes the deprecation, not from the first commit that includes the deprecation.

finding and removing deprecated code isn't any more or less work whether you do it sooner or later.

There is some overhead to having deprecated code hanging around the codebase though, and the bookkeeping to keep track of which stuff can be removed when is overhead too. (The more times we go "hey can we remove this? Oh not yet", the more attention we're wasting.) It's not necessarily a big deal, but like here we've just had a contributor's PR closed because of it; Laura is doing extra work because of this policy right now!

semver

For this project in particular, our challenge is to become a stable foundation library as quickly as we can. I would love it if we could release a 1.0 within, say, a year. But to do this we need more users to gain confidence in our API, which means that between now and 1.0 we'll need to do a juggling act where as more users gradually come on board, we gradually get stricter about stability so that the next round of users feels comfortable joining us... So it's a little more complex than "semver says that anything goes".

@jab
Copy link
Member

jab commented Apr 16, 2018

...between now and 1.0 we'll need to do a juggling act... So it's a little more complex than "semver says that anything goes".

Well put. With the semver reference I only meant to point out that you're covered there, in case that's ever helpful, and also that (maybe many more) Trio users (than you'd think) would support and trust you in making breaking changes now as you make progress toward 1.0. (At least I would, and my previous comment has been 👍'd by someone else already, fwiw.)

@smurfix
Copy link
Contributor

smurfix commented Apr 16, 2018

server
semver ;-)

like here we've just had a contributor's PR closed because of it;
Laura is doing extra work because of this policy right now!

Except that, well, the PR had other issues …

As the release manager, I'm prepared to do that go-through-deprecations work.

Is everybody OK with setting that timer to a month, for now?

@smurfix
Copy link
Contributor

smurfix commented Apr 16, 2018

@Fuyukai even if this particular PR didn't work out, thanks for doing busywork like going through issues that may or may not be closed by now!

@njsmith
Copy link
Member

njsmith commented Apr 16, 2018

Is everybody OK with setting that timer to a month, for now?

Works for me. Let's move the discussion over to #500.

@Fuyukai Fuyukai deleted the system-tasks-contextvars branch April 20, 2018 20:32
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 this pull request may close these issues.

4 participants