voila-dashboards / voila-vuetify

Dashboard template for Voilà based on VuetifyJS
Other
154 stars 41 forks source link

drop voila max dependency requirement #58

Closed keykey7 closed 2 years ago

keykey7 commented 2 years ago

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

martinRenou commented 2 years ago

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

maartenbreddels commented 2 years ago

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

martinRenou commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 :smiley: ).

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 commented 2 years ago

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

martinRenou commented 2 years ago

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 commented 2 years ago

It basically comes down to this:

keykey7 commented 2 years ago

I reintroduced the upper bound, can we merge this?

maartenbreddels commented 2 years ago

@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 https://github.com/jupyter/nbconvert/pull/1741#issuecomment-1086450082 https://iscinumpy.dev/post/bound-version-constraints/ is a good read on this.

mariobuikhuizen commented 2 years ago

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 commented 2 years ago

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

martinRenou commented 2 years ago

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 commented 2 years ago

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

maartenbreddels commented 2 years ago

I plan to defend breakage :)

iynehz commented 2 years ago

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

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 commented 2 years ago

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?