nengo / nengo-gui

Nengo interactive visualizer
Other
97 stars 38 forks source link

Set minimum and maximum Nengo versions. #1001

Open jgosmann opened 5 years ago

jgosmann commented 5 years ago

Current master branch requires at least Nengo 2.8 because it is importing

from nengo.utils.compat import escape

This PR sets Nengo requirement in the setup file accordingly.

jgosmann commented 5 years ago

(reported here)

arvoelke commented 5 years ago

Note nengo.utils.compat was deprecated in Nengo version 3.0.0-dev and is scheduled to be removed upon the next minor release thereafter (e.g., 3.0.1 3.1.0).

jgosmann commented 5 years ago

Note nengo.utils.compat was deprecated in Nengo version 3.0.0-dev and is scheduled to be removed upon the next minor release thereafter (e.g., 3.0.1).

I thought Nengo follows semantic versioning (mostly)? The removal is definitely a breaking change and should not happen in a minor increment and definitely not in a patch increment. (So the proper path to me seems to make another minor 2.x release with the deprecation and then remove it in 3.0.)

But apart from that, we could also set a maximum Nengo version or move the relevant imports to nengo-gui.

arvoelke commented 5 years ago

Sorry by minor release I meant e.g., 3.1.0. Not a patch increment. Details here under "deprecated". Does this seem reasonable?

jgosmann commented 5 years ago

Depends on what meaning you assign to the version numbers. But removing it in a minor version contradicts Semantic Versioning which Nengo is supposedly using: https://www.nengo.ai/governance.html

arvoelke commented 5 years ago

Yeah, so you are technically right, but at the time the team decided that it's not worth following the semver spec exactly for cases such as these. Since the dev branch is already at 3.0.0-dev due to backwards-incompatible changes, and will be released with the deprecation notice, this would require us to hang on to this untested / dead code (note we are no longer testing or supporting Python < 3) for an entire major version, or release a version 4 soon after just to get rid of this. Current use of nengo.utils.compat should be pretty sparse and limited to the Nengo ecosystem and so we don't see it as a serious concern.

jgosmann commented 5 years ago

or release a version 4 soon after just to get rid of this

What is the problem with that? The way I see it is that the purpose of Semantic Versioning is to give you a reliable contract that you can depend on to specify version ranges for dependencies. If you are worried how it looks to the user or want to decide on version increments by how "big" or impactful the changes are, that's fine too, but maybe it shouldn't say that Nengo is using Semantic Versioning?

Or I suppose I could see an argument that nengo.utils.compat is not considered part of the public API (and Semantic Versioning only applies to that part of the API). However, in that case, why not just remove it without the deprecation step?

Also, it is much more critical if you intend to remove the deprecated SPA module and network inputs in a minor release because these are part of the public API.

tbekolay commented 5 years ago

nengo.utils.compat is not considered part of the public API (and Semantic Versioning only applies to that part of the API)

I would agree with this, I don't consider the things in nengo.utils to be part of the public API. The deprecation step is mostly a convenience for downstream Nengo projects so we don't have to necessarily update them all before making the 3.0.0 release.

arvoelke commented 5 years ago

What is the problem with that?

IMO it would make the versioning less meaningful if the only difference between 3 and 4 was removing an internal API that was only kept around to help smoothen the transitioning our own projects. Meanwhile the difference between version 2 and 3 is quite enormous, spanning multiple years of development.

However, in that case, why not just remove it without the deprecation step?

Mostly as a courtesy / convenience for ourselves.

I'll also just point out that these sorts of decisions to deviate from the semantic versioning spec are fairly common in some of the most popular / depended upon packages that claim to use semantic versioning. For example, pytest frequently removes deprecated features even on patches. IMO it's about finding the right balance between all these considerations, and sometimes this doesn't strictly follow the letter of the law.

jgosmann commented 5 years ago

it would make the versioning less meaningful

That depends on what meaning one assigns to the version number. Is it about the amount of changes (by some measure, which seems the meaning Nengo assigns to some degree) or whether changes are breaking, adding features, or fixing bugs (the meaning that Semantic Versioning assigns). The former one might be more useful from a user perspective, the latter when trying to define a sensible version range for a dependency.

pytest frequently removes deprecated features even on patches

On a quick skim I could not find any removals on patch increments and the removals of deprecated features seem to be features that have been deprecated in the previous major version (and maybe were simply forgotten to be removed in the x.0.0 release).

But I guess it is not helpful to spend much more time on this discussion, but rather figure out what that means for this PR. Even though you are not strictly following Semantic Versioning, I suppose that a major version increment should be considered breaking. Thus, would it make sense to define the required Nengo version by Nengo GUI as >=2.8,<3? Or would you prefer >=2.8,<3.1 (and can you ensure that this would not break)?

This would be a quick and easy fix that ensures that a user gets a working install and IMO something like this should be done and released as a a first measure.

Then, should Nengo GUI be made compatible to Nengo 3 and if so by dropping Python 2.7 (and thus Nengo <3 support) or by moving the escape import code to Nengo GUI itself (thus supporting Nengo >2.8 including 3)?

Aside from the GUI, the same questions would apply to Nengo SPA.

arvoelke commented 5 years ago

The former one might be more useful from a user perspective, the latter when trying to define a sensible version range for a dependency.

I'm advocating for a reasonable balance between the two.

Thus, would it make sense to define the required Nengo version by Nengo GUI as >=2.8,<3?

That is what I did here: https://github.com/arvoelke/nengolib/commit/1f3832c6cfca89097495d61eb838b975fceeb972, and then my plan was to have nengolib==1.0.0 support nengo>=3.0.

jgosmann commented 5 years ago

Added a commit to set the upper Nengo version too.