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

Consider adding lower bounds to nbconvert #645

Open
matthewfeickert opened this issue Apr 4, 2022 · 2 comments
Open

Consider adding lower bounds to nbconvert #645

matthewfeickert opened this issue Apr 4, 2022 · 2 comments

Comments

@matthewfeickert
Copy link

matthewfeickert commented Apr 4, 2022

I'm hesitant to add a lower bound to nbconvert because old versions of nbconvert work perfectly fine with old versions of jinja2. It's only a certain combination of versions that doesn't work.

Hm, maybe I'm missing your point here, but this seems to be the exact reason that I would suggest lower bounds. Keeping lower bounds that are regularly tested and updated allows you to update without breaking users, as if they have restrictions on the dependencies of nbsphinx the environment solver would then know to roll back to an older version of nbsphinx that can support that restriction.

nbconvert has quite a few versions out on PyPI

$ python -m pip index versions nbconvert
WARNING: pip index is currently an experimental command. It may be removed/changed in a future release without prior warning.
nbconvert (6.4.5)
Available versions: 6.4.5, 6.4.4, 6.4.3, 6.4.2, 6.4.1, 6.4.0, 6.3.0, 6.2.0, 6.1.0, 6.0.7, 6.0.6, 6.0.5, 6.0.4, 6.0.3, 6.0.2, 6.0.1, 6.0.0, 5.6.1, 5.6.0, 5.5.0, 5.4.1, 5.4.0, 5.3.1, 5.3.0, 5.2.1, 5.1.1, 5.1.0, 5.0.0, 4.3.0, 4.2.0, 4.1.0, 4.0.0
  INSTALLED: 6.4.5
  LATEST:    6.4.5

but the only constraint on it from nbsphinx is that

'nbconvert!=5.4',

So does nbsphinx at HEAD of 80d9b26 work with the nbconvert all the way back to v4.0.0 with the singular exception of v5.4.0? If not, it would be important to reflect this in the install_requires so that solvers know where to impose restrictions.

I don't know if that's possible to express in conf.py.

No, conf.py can't enforce version restrictions as that's the user's responsibility to setup a working environment.

If I'm missing the point here I apologize, as I'm not trying to be annoying.

Original motivation for Issue posted by @mgeier in #641 (comment)

@mgeier
Copy link
Member

mgeier commented Apr 5, 2022

Just to make sure there's no misunderstanding: You are suggesting to add nbconvert>=6.4.5 to install_requires, right?

[...] if they have restrictions on the dependencies of nbsphinx the environment solver would then know to roll back to an older version of nbsphinx that can support that restriction.

But I'd like them to be able to use the latest nbsphinx. Otherwise it's an obstacle for contributing.

And why should nbsphinx be downgraded for something that isn't its fault?

So does nbsphinx at HEAD of 80d9b26 work with the nbconvert all the way back to v4.0.0 with the singular exception of v5.4.0?

Yeah, I'd expect that. But I haven't tested it, so it's probably not the case.
But at least nobody has reported a broken version.

I don't know if that's possible to express in conf.py.

No, conf.py can't enforce version restrictions as that's the user's responsibility to setup a working environment.

Sorry, I mistyped!
I meant setup.py!

I meant if it's possible to defined a dependency like this:

  • if jinja2 is older than 3.1.0: no restrictions on nbconvert, everything is fine
  • if jinja2 is newer, nbconverthas to be at least 6.4.5

I guess not.

If I'm missing the point here I apologize, as I'm not trying to be annoying.

No, not at all! I appreciate your suggestions!

I'm far from a packaging expert and I'm still learning, but in the last few years I've learned at least one thing: it's complicated.

In general I try not to impose too many restrictions on versions, because users can easily add their own restrictions, but they cannot that easily remove my restrictions.

Also, I'm not so sure if this will affect anyone from now on, who hasn't been affected already. I think all new installations will work, the problem is only with users who have updated their packages before nbconvert 6.4.5 or are forced to use an old version for some other reason.
Are those a lot of people?

Having said that, if there is a concrete enough reason to add a lower bound, I'm still open to hearing it.

I guess the question is hard to answer: will it help more people than it harms?

@matthewfeickert
Copy link
Author

Just to make sure there's no misunderstanding: You are suggesting to add nbconvert>=6.4.5 to install_requires, right?

Not necessarily and probably not. I would more specifically suggest to emperically evaluate the lower bounds for all the core dependencies of nbsphinx with the oldest CPython nbshpinx supports (currently Python 3.6), and then ideally also put these as pins in a constraints.txt file and use them to nightly/weekly test the nbsphinx API requirements.

As tests run in CI will have the core dependencies unconstrainted, then you're naturally already testing the extremes of the new APIs that come up. If those require you to alter the nbsphinx API in a way that causes the lower bound constraint tests to fail, then you know that you need to update your lower bounds.

This all, along with not setting uppper bounds, ensures that users will have the best chance of their dependency solver giving them a working environment upon install. If a new version of any of nbsphinx's dependencies breaks things, then they can always try to roll back those dependencies, but nbsphinx's lower bounds will keep them from getting a version that will break the nbsphinx API.

So my guess it that nbconvert>=6.4.5 is not the lower bound, but this approach would find and ensure a lower bound compatible with the API.

I fully appreciate that emperically finding out what are the minimum lower bounds that work with your API is a slow and uninteresting process (I've done it) and it is totally reasonable to say that it isn't a high enough priority to warrant core dev time right now.

But I'd like them to be able to use the latest nbsphinx. Otherwise it's an obstacle for contributing.
And why should nbsphinx be downgraded for something that isn't its fault?

Hm. I'm not sure how this is an obstacle for contributing. Here I was saying that a user has an explicit requirement that they need an older version of a core dependnecy of nbsphinx then the current nbsphinx API requires. If there is no lower bound then this will get installed and everything will break. If there is a lower bound for nbshpinx then a working environment can be installed, and if that older constraint ever gets lifted then a newer version of nbsphinx can get installed no problem.

Yeah, I'd expect that. But I haven't tested it, so it's probably not the case. But at least nobody has reported a broken version.

If so, that's great and very impressive! 👏

Sorry, I mistyped! I meant setup.py!

I meant if it's possible to defined a dependency like this:

  • if jinja2 is older than 3.1.0: no restrictions on nbconvert, everything is fine
  • if jinja2 is newer, nbconverthas to be at least 6.4.5

I guess not.

Yeah, it isn't possible to do staged dependency resolution like this. What you're currently doing with disallowing specific versions of a dependency that are known to have a problem that breaks everything is the best thing.

'nbconvert!=5.4',

Though all the suggestions that I'm making here I should also be making to nbconvert, so that nbsphinx gets the same advantages that I was describing for your users. Note that nbconvert is effectively enforcing a lower bound on jinja2 for nbsphinx already, which is a good think to explicitly know.

I'm far from a packaging expert and I'm still learning, but in the last few years I've learned at least one thing: it's complicated.

Same. Here are three blogs that I have found to be truly helpful in helping me understand a lot of this better for the packages I maintain.

In general I try not to impose too many restrictions on versions, because users can easily add their own restrictions, but they cannot that easily remove my restrictions.

👍 This is why lower bounds are important and equally why upper bounds should really be avoided.

I guess the question is hard to answer: will it help more people than it harms?

My response is pretty rambly, but in my mind setting lower bounds can only help. It might make some users realize that they have been installing slightly broken environments this whole time, but it will also help them fix them and guard them from breaking in the future.

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

No branches or pull requests

2 participants