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

drop voila max dependency requirement #58

Merged
merged 2 commits into from
Jul 26, 2022

Conversation

keykey7
Copy link
Contributor

@keykey7 keykey7 commented Apr 1, 2022

as voila-vuetify seems to work fine with voila 0.3.x

@martinRenou
Copy link
Member

Thanks a lot! This should probably be changed to <0.4 and not removed, would you be able to update your PR?

setup.py Outdated Show resolved Hide resolved
@maartenbreddels
Copy link
Member

I think we should drop it, I find upper boundaries hurt more than they help.

@martinRenou
Copy link
Member

I find upper boundaries hurt more than they help.

Why do you think upper boundaries hurt?

We've seen what happens when upper bounds are not there...

@maartenbreddels
Copy link
Member

You cannot upgrade now without maintainers doing upgrades to their requirements. Also, we cannot use older versions of voila-vuetify with newer versions of voila now, even though they might work, and they probably work.
I think breaking changes should always have minimal breaking changes (e.g. behaviour), not things like renaming and such (like what happened with Jinja)

@martinRenou
Copy link
Member

Also, we cannot use older versions of voila-vuetify with newer versions of voila now, even though they might work, and they probably work

I think this is very minor inconvenience over having everything broken from time to time (like what happened with Jinja, jupyter-client 7, JupyterLab 3, what will happen with Jupyter Notebook 7 and the list will go on and on)

@maartenbreddels
Copy link
Member

I think we disagree there. I think there is no right decision btw, just tradeoffs, I think a lot of that pain can be avoided (like Jinja), but also pinning comes with pain.

@keykey7
Copy link
Contributor Author

keykey7 commented Apr 1, 2022

The reason for this PR is that poetry does not have a possibility to force-ignore such transitive dependency constraints. So the only way is a cumbersome library bump (this PR 😃 ).

On the other hand, if voila-vuetify wouldn't have an upper bound and latest voila would fail, "downgrading" this transitive dependency would have been trivial. Therefore I removed it, but both views make sense.

@maartenbreddels
Copy link
Member

If every tool did upper bound limits, we couldn't install/upgrade anything within a year :)

@martinRenou
Copy link
Member

Thumb up on the fact we disagree there.

I agree it's just tradeoffs, and I personally prefer to see people complain because they cannot upgrade (which is easily fixed) to people complain because their setup gets broken suddenly just because a low-level lib made a backward incompatible release.

@martinRenou
Copy link
Member

It basically comes down to this:

  • no upper-bound: users have to understand why their packages stop working suddenly and need to figure out which package they need to upgrade/downgrade to get back a working setup.
  • upper-bound: developers have to manage the constraints and make sure those constraints are up to date, users will always have a working setup.

as discussed in voila-dashboards#58

Co-authored-by: martinRenou <martin.renou@gmail.com>
@keykey7
Copy link
Contributor Author

keykey7 commented Apr 3, 2022

I reintroduced the upper bound, can we merge this?

@maartenbreddels
Copy link
Member

@mariobuikhuizen do you want to weigh in on this?

I don't think semver adds much for these things, a 'fix' release can break something, a 'major' release may break nothing.

Reposting what @blink1073 posted at jupyter/nbconvert#1741 (comment)
https://iscinumpy.dev/post/bound-version-constraints/ is a good read on this.

@mariobuikhuizen
Copy link
Member

I'm still in doubt. I agree it's terrible for a user if one of their dependencies has an upper bound and prevents upgrading potentially an entire chain of dependencies. On the other hand, if it's a library under your control and the upper bound is updated in a timely manner after one of its dependency has a new version, it could work.

But come to think of it, voila-vuetify relies on some of Voilà's internals which could have breaking changes for us on minor versions, so the upper-bound in this case just gives a false sense of security.

@blink1073
Copy link

blink1073 commented Apr 3, 2022

There is a provision for applications with tightly coupled dependencies. For example, jupyterlab pins its server version.

@martinRenou
Copy link
Member

On the other hand, if it's a library under your control and the upper bound is updated in a timely manner after one of its dependency has a new version, it could work.

👍🏽 I believe it's sane to keep an upper bound on Voila which we have control over.

@maartenbreddels if you really want to remove it I won't block that change, but please don't willsmith-slap me if I push changes on Voila for a major version that breaks voila-vuetify 😄

@keykey7
Copy link
Contributor Author

keykey7 commented Apr 9, 2022

...sooo, we can merge this before they introduce voila 0.4 and break everything again? 😃

@maartenbreddels
Copy link
Member

I plan to defend breakage :)

@iynehz
Copy link

iynehz commented Jun 20, 2022

Hi, could you just merge this <0.4 upper bound and cut a new release? So at least you restore the compatibily with latest voila 0.3.x.

btw, IMO whether or not have upper bound requirement generally depends on

  • what apis you use from your dependency
  • how your dependency's owner/maintainer understands semver and versions their product.

If as @mariobuikhuizen mentioned that voila-vuetify depends on some voila internals, IMHO you better still keep the your upper bound to the minor version level. Actually, with the status of open source python community, setting upper bound to the minor version level is generally the safest and most balanced way.

@maartenbreddels maartenbreddels merged commit 84946fc into voila-dashboards:master Jul 26, 2022
@maartenbreddels
Copy link
Member

It's at least better, sorry for leaving this hanging for so long.
@mariobuikhuizen do you have time to make a release, or automate the releasing of version bump like we do with react-ipywidgets?

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.

None yet

6 participants