Requiring tornado<5

[SylvainCorlay]

It seems that tornado 5 breaks notebook, and we should require tornado<5 in the recipe.

Kind of errors that people have encountered with tornado 5:

Traceback (most recent call last):
  File "/Users/.../anaconda3/envs/cling/bin/jupyter-notebook", line 4, in <module>
    import notebook.notebookapp
  File "/Users/.../anaconda3/envs/cling/lib/python3.6/site-packages/notebook/notebookapp.py", line 45, in <module>
    ioloop.install()
  File "/Users/.../anaconda3/envs/cling/lib/python3.6/site-packages/zmq/eventloop/ioloop.py", line 210, in install
    assert (not ioloop.IOLoop.initialized()) or \
AttributeError: type object 'IOLoop' has no attribute 'initialized'

[takluyver]

People encountering that error should upgrade pyzmq.

The requirement is essentially tornado<5 if pyzmq<17, but dependencies aren't flexible enough to express that. The notebook works with tornado 5 if people have updated other packages.

[SylvainCorlay]

• OK, this breaks our QuantStack build since we need a gcc-7 build of everything linked with xeus-cling, including pyzmq, and QuantStack is meant with a higher priority than conda-forge. We could fix this by updating QuantStack's pyzmq.
• More generally, I think that it would be a reasonable policy to not have open-ended >= requirements for dependencies.

[SylvainCorlay]

@minrk not sure this sould have been closed: maybe we should add <6 to the tornado dependency.

The ideal scenario would have been the previous build to require

pyzmq>=16.0,<17
tornado>=4.0,<5

and the next build to do

pyzmq>=17.0,<18
tornado>=4.0,<6

etc...

[minrk]

Should we then apply semantic assume-major-release-breakage versioning on all dependencies throughout conda-forge? I'm not sure tornado and pyzmq should get special treatment and not traitlets, jupyter-client, jinja, terminado, etc.

I think we shouldn't pin pyzmq (pyzmq releases have never broken notebook, I believe), and probably shouldn't pin tornado, either. There has been exactly one release of tornado that caused a breakage and we had releases of notebook and other packages that fixed compatibility before the tornado release was public, so if you have current packages, nothing would break.

I think it's important to note that the current situation you are hitting would not have been avoided in any way by adding these pinnings, since the current version of the notebook and all packages on conda-forge do and should support tornado 5. Only the effective out-of-date pinning of pyzmq on QuantStack caused the breakage, which cannot be solved by any dependency pinning available to us.

[SylvainCorlay]

Only the effective out-of-date pinning of pyzmq on QuantStack caused the breakage, which cannot be solved by any dependency pinning available to us

In fact, it is just the mechanism of channel priorities. We just did not take the time to package 17 yet...

So there was no deliberate pinning on our side.

[SylvainCorlay]

Should we then apply semantic assume-major-release-breakage versioning on all dependencies throughout conda-forge? I'm not sure tornado and pyzmq should get special treatment and not traitlets, jupyter-client, jinja, terminado, etc.

Yes, that is what I meant, at least require <MAJOR when MAJOR is not out. We can always make new builds with broader requirement ranges later.

I'm not sure tornado and pyzmq should get special treatment and not traitlets, jupyter-client, jinja, terminado, etc.

Right, I meant it for all packages.

[minrk]

Yeah, that's why I said "effective" pinning, because it wasn't intentional, but the result was the same. Absolutely no amount of dependency pinning could have solved this because pyzmq and tornado have no dependency relationship and there's no way for pyzmq to say "I don't require tornado, but I'm compatible with tornado X.Y if you have it". So while we could apply this pinning here, we should be clear that it would have no bearing on fixing similar issues as they crop up in the future.

[SylvainCorlay]

That is right, and why this can be fixed in packages requiring both, such as notebook.

[takluyver]

Not really, though, because notebook is compatible with tornado 5, so we would already have removed the <5 requirement by now.

[SylvainCorlay]

Notebook is not compatible with all combinations of pyzmq and tornado with the current constraints. It has the opportunity to fix it.

Earlier builds should have had tornado <5.0 and new ones could have relaxed this to <6.0 by also requiring pyzmq>17.

[SylvainCorlay]

@minrk @takluyver would you like to have a quick chat over appear.in?

[minrk]

by also requiring pyzmq>17.

This would be the wrong thing to do, because notebook works with pyzmq >= 13, I believe. It is only the specific combination of tornado 5 and pyzmq < 17 that doesn't work. It would be wrong for notebook to require pyzmq 17.

[SylvainCorlay]

Since conda cannot express the exact set of combinations for which thing work, It is then a debate between - a strict subset of all working combinations - a strict superset of all working combinations. I prefer the first possibility which seems more conservative to me. Note that with my proposal, two subsequent builds of the same version could together represent all the working ones.

…

On Sat, Mar 31, 2018, 22:06 Min RK ***@***.***> wrote: by also requiring pyzmq>17. This would be the wrong thing to do, because notebook works with pyzmq >= 13, I believe. It is only the *specific combination* of tornado 5 and pyzmq < 17 that doesn't work. It would be wrong for notebook to require pyzmq 17. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#31 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACSXFnseCgmgjnxqn5360K_JUJnXsrsrks5tj-GogaJpZM4S-PMt> .

[minrk]

It's more conservative as long as pyzmq doesn't introduce any issues itself. If pyzmq 17 (which has major reworking of its eventloop integration) causes problems, it becomes impossible to downgrade pyzmq to isolate the problem because of a totally artificial dependency limitation.

[takluyver]

I don't think it's worth trying to solve this kind of problem when we've only had one instance of the problem so far (the upgrade to tornado 5). Adding more constraints on dependencies means more maintenance work, and we have a lot of packages to maintain.

Where packages follow semver clearly enough that a major version bump will likely mean incompatibilities, I think it's reasonable to specify a <5 requirement until we've adapted our code to the new version. But as far as I recall, the move from tornado 3 to 4 went pretty smoothly, so I don't know if we'd pin tornado like that.