-
Notifications
You must be signed in to change notification settings - Fork 17
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
Made compatible with Zookeeper >= 0.5 #62
Conversation
setup.py
Outdated
@@ -8,7 +8,7 @@ def readme(): | |||
|
|||
setup( | |||
name="larq-zoo", | |||
version="0.3.0", | |||
version="0.4.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if this is right. Since this is breaking (in that the new zookeeper version is incompatible with the old one) I assume we go from 0.3.0 to 0.4.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like so far each release had either some new model or improved weights of already existing models in it. For my feeling this is more a 0.3.1 but I am also a bit uncertain about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@koenhelwegen could you shine some light here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to change the version number when you change the code - it's fine if the master branch is non-compatible with the latest release. Releases are used to make a specific version of the code easily available (e.g. after a bug fix or when several changes are made). This is done by going to releases and drafting a release. When the time is there, we make a one line PR to change the version number (example).
Once we make the release I think we may want to go to 0.4 as it's a breaking change, but below 1.0.0 there are no real rules in general as far as I know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. There is no need to change the version number here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reset to 0.3.0
@@ -17,7 +17,7 @@ def readme(): | |||
url="https://github.com/plumerai/larq-zoo", | |||
packages=find_packages(), | |||
license="Apache 2.0", | |||
install_requires=["numpy~=1.15", "larq~=0.5", "zookeeper>=0.3.1,<0.5.0"], | |||
install_requires=["numpy~=1.15", "larq~=0.5", "zookeeper~=0.5"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't it require zookeeper 0.5? If so I would use >=0.5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does, I have implemented your fix in 7919417
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should keep the dependency fixed to zookeeper~=0.5
to prevent us from accidentally pulling in breaking changes.
For reference ~=0.5
is equivalent to >=0.5.0,<0.6.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reset to ~=0.5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I have a few comments though.
7919417
to
d529090
Compare
This fixes #57 closes #55