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

--ExecutePreprocessor.timeout default would be more sensible to be None #791

Closed
jpz opened this issue Apr 1, 2018 · 12 comments
Closed

--ExecutePreprocessor.timeout default would be more sensible to be None #791

jpz opened this issue Apr 1, 2018 · 12 comments

Comments

@jpz
Copy link

jpz commented Apr 1, 2018

I see this was discussed previously here: #256 where it appeared there was some agreement amongst some participants that having a timeout as the default may be surprising behaviour and the alternative might be preferable.

Arguing, on the other hand, was the point of view that as nbconvert produces no streamed output, so the timeout was introduced to improve feedback to users.

My concern is, that having just encountered this timeout for the first time, was how inconsistent nbconvert --execute seems to be, compared to running the same notebook interactively. It took me very much by surprise.

If a user was able to run an interactive notebook ran without issue, I would imagine that they would expect nbconvert --execute to run without issue, but rather what they may encounter is a non-deterministic timing issue.

One can imagine other scenarios related to post-release also, where hardware will be different from test or dev environments where timings will be different. It could be entirely likely to think you have successfully deployed, only to see the 30 sec barrier hit on one of your overnight batch processes for instance, as you were simply unaware this timeout was present.

Under those use cases, I would argue that opting-in rather than opting-out may be the better choice.

Of course, given the very large deployment footprint of jupyter these days, I'm sure functional changes like this are not always reasonable to request or pragmatic to make - but I just wanted to give my feedback in the case that it had value in the furtherance of the quality of the project.

@mgeier
Copy link
Contributor

mgeier commented Apr 4, 2018

@Carreau said in #256 (comment):

I doubt we would disable it by default as the actual behavior was added for people that were actually annoyed it did not stop.

It would be nice to get more details about that or some links to previous discussions.

I'm having a hard time imagining a situation where I would be rightfully annoyed that something that takes a long time takes a long time.

I guess the only situation where this argument would make any kind of sense would be if the execution "hangs" (forever), but how often does that happen?
And how often does that not happen when running the notebook interactively but does happen when running with nbconvert?
AFAICT this can only happen if there is a bug somewhere. This would mean that the current default value is chosen to support buggy code, while causing seemingly random errors on perfectly correct code.
IMHO this is not a good way to choose default values.

Given only the information in #256 and here in this issue, I agree with @jpz that running to completion without a timeout makes the most sense as a default.

The main argument IMHO is that it is quite possible to happen that a notebook runs quicker than the timeout in interactive use but it reaches the timeout when run with nbconvert (on a different computer).
It is probably even more likely that someone prepares some notebooks to be run by nbconvert (and they run fine), but when they are given so somebody else (with a less powerful computer), they fail and cause a lot of totally unneeded confusion.

@takluyver
Copy link
Member

I don't feel strongly about this. If we were writing new code, I agree it probably makes more sense to have no timeout by default. But I went digging, and there has been a time limit on by default for as long as the execute preprocessor has been in our code (it was added in ipython/ipython#6053 ).

Since every change breaks someone's workflow, and the potential problems if you rely on the default are quite severe, it might be easiest to leave it as is. If we do decide to change it, I think we should go through a deprecation cycle - i.e. print warnings for anyone using the default timeout that they should set it explicitly if they want to still have a timeout in the future.

@mgeier
Copy link
Contributor

mgeier commented Apr 8, 2018

Thanks @takluyver for acknowledging that this is indeed a bug!

there has been a time limit on by default for as long as the execute preprocessor has been in our code

Yes, it seems to me that's happening too often in the Jupyter project, you should probably talk about this in the Steering Committee.

It seems to be much easier to introduce bugs together with new features, than to fix bugs after they are discovered. It sometimes seems that you (i.e. the core developers) don't even want to fix bugs.

I think you should change your policy in one of these two ways:

  • stronger code review
  • be more open to "breaking" changes caused by bug fixes

I think the first point would hurt the project more than the second point.
And doing nothing will hurt the project in the long run, because it will become buggier and buggier.

the potential problems if you rely on the default are quite severe, it might be easiest to leave it as is.

What are those problems?

Nobody has mentioned any concrete problems yet!

I think we should go through a deprecation cycle - i.e. print warnings for anyone using the default timeout that they should set it explicitly if they want to still have a timeout in the future.

I don't think that's reasonable.
Because we cannot distinguish whether somebody is using the default value intentionally and consciously or if they just don't think about timeouts (which I guess will be about 99% of the cases).

Unless somebody comes up with a compelling case where this would affect any user negatively, we should indeed change the default for the sake of those real cases that @jpz is describing.

@takluyver
Copy link
Member

I didn't and wouldn't call it a bug. It's a difference of opinion on what the most sensible default is. I'm agreeing with you that the current default is probably not the best. But I'm wary of changing it, because consistency is also valuable: changing what people are used to can always break things, even if you think the change makes it better.

the potential problems if you rely on the default are quite severe, it might be easiest to leave it as is.

What are those problems?

Oh, sorry. The problem is for anyone using nbconvert to execute notebooks in an automated pipeline: if there's no timeout, any notebook which gets stuck in an infinite loop, or tries to do a really long operation, blocks up your pipeline. That might just mean that later processing stages never happen, or it might eat up CPU or memory. Because the default is currently to stop after 30 seconds, people could easily be relying on this without ever having thought about it.

I maintain that if we do change the default, we should go through a deprecation period first.

@jpz
Copy link
Author

jpz commented Apr 8, 2018

I am extremely cautious about diving in and changing things - you know what will break. Any widely adopted software needs to move cautiously. So I support that.

I would not call it a bug either, it’s a design decision that was made with the best of intent by some people that contributed some great software to us all. Feedback on the design is only meant in the spirit of discussing the design and specification, and where things can be better.

Given that, I agree that the idea of deprecating the behaviour is a good idea to consider, IMO.

I don’t think the behaviour is self-evidently good, as it will be surprising to people as a problem they need to fix in their notebook - and it makes batch processing inhomogenous to running it interactively (which will not timeout after 30sec.)

@takluyver
Copy link
Member

Thanks @jpz :-) I'm going to ping the Jupyter mailing list to see if other people have input on this decision.

@BjornFJohansson
Copy link

BjornFJohansson commented Apr 10, 2018

Came here because of the email in the Jupyter mailing list. I think disabling cut-off seems sensible by default. If or when it is needed if could be re enabled. When I hit the limit, it was usually du to bugs in my code.

@guenter
Copy link

guenter commented Apr 20, 2018

I recently ran into the timeout and it caught me by surprise since I was used to running cells for > 30s in the UI. My vote would be for making the behavior consistent between UI and nbconvert to avoid surprises.
While I generally try to avoid changing defaults I think the effects here aren't terrible and can be minimized with a message on startup when the setting isn't changed by the user.
Another consideration here is new vs. existing users. The current behavior is surprising to new users and I'd argue that only a small-ish subset of existing users will notice the change. Jupyter is a fast growing project so I'd optimize for the future majority.

@carlthome
Copy link

This just confused me a lot too.

@mgeier
Copy link
Contributor

mgeier commented Dec 4, 2018

Since @takluyver posted to the mailing list (https://groups.google.com/forum/#!topic/jupyter/XUDcijnSsuM) there were a few more "votes" for changing the default, and zero comments against (if I didn't misinterpret some comments).

Shall we proceed with changing the default?

@mgeier
Copy link
Contributor

mgeier commented Feb 13, 2019

It's been a while ... there have been no comments against making the suggested change, AFAICT.

So let's do it, shall we?

Probably someone should just make a PR for this?

@mgeier
Copy link
Contributor

mgeier commented Sep 10, 2020

This has finally been fixed in nbconvert 6!

This issue can be closed.

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

No branches or pull requests

8 participants