8000 MNT use x.y.z (no rc/beta/etc) for version in sphinx config by adrinjalali · Pull Request #15566 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]

Skip to content

MNT use x.y.z (no rc/beta/etc) for version in sphinx config #15566

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

Merged
merged 4 commits into from
Nov 11, 2019

Conversation

adrinjalali
Copy link
Member

We expect the version to be x.y.z (no rc/beta/etc), and release to include the beta/rc/etc info. This PR takes the base part of the __version__ for version variable in sphinx config.

Note that the regexp checkng for this had been introduced in #11221

Alternatively we can fix that regexp.

@adrinjalali
Copy link
Member Author

Ping @jnothman @lesteve

Copy link
Member
@jnothman jnothman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The binder stuff below requires 'dev' to be identifiable in version. But it should probably rather be using release.

@lesteve
Copy link
Member
lesteve commented Nov 8, 2019

I did the simplest thing at the time. I agree this is not very robust and this may cause complications with the beta release.

You may have discussed this kind of things already elsewhere: rather than distutils.version ideally we would use pkg_resources.parse_version and .is_prerelease (needs setuptools IIRC) or even packaging.version.parse (needs packaging package). The latter makes the difference between a dev and a pre-release.

Also the .binder/requirements.txt in 0.22.X needs to be tweaked once 0.22 is released, i.e. you need to remove these two lines:

--find-links https://sklearn-nightly.scdn8.secure.raxcdn.com
10000
 scikit-learn
--pre

There could also be an intermediary step where you install 0.22b1 in binder.

'Ill-formed version: {!r}. Version should follow '
'PEP440'.format(version))

if v.is_devrelease:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I am missing something, but I think pkg_resources.parse_version does not return an object with a .is_devrelease attribute. On the other hand, packaging.version.parse does.

In [1]: from packaging.version import parse                                                                                                                                        

In [2]: parse('0.22.dev0').is_devrelease                                                                                                                                           
Out[2]: True

In [3]: from pkg_resources import parse_version                                                                                                                                    

In [4]: parse_version('0.22.dev0').is_devrelease                                                                                                                                   
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-4-13d9ab197353> in <module>
----> 1 parse_version('0.22.dev0').is_devrelease

AttributeError: 'Version' object has no attribute 'is_devrelease'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rather not have a dependency just to parse the version. And the issue is that the new pkg_resources actually do have the is_devrelease attribute, and it works fine. The older ones however, don't. Trying to figure out when it was added to it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, setuptools actually ships a version of packaging with it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♂️ setuptools ships packaging=16.8, whereas Version().is_devrelease was introduced in packaging=17.0. It worked on my system because pkg_resources uses the installed lib instead of the vendored one if it finds it, and I had a new packaging in that environment.

@adrinjalali
Copy link
Member Author

all green \o/

But now we're installing packaging for doc build.

@adrinjalali
Copy link
Member Author

For now this seems to be the only blocker for an RC by the way.

Copy link
Member
@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we will add a packaging dependency for building the docs, which can be removed if setuptools update its dependency?

I am okay with this. Thank you @adrinjalali !

@adrinjalali
Copy link
Member Author

@jnothman @lesteve you okay with this one? Would really like to get an RC out today or tomorrow.

Copy link
Member
@jnothman jnothman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmm... Okay. I'd rather avoid the dependency but it's maybe no big deal

@adrinjalali
Copy link
Member Author

Hmmmm... Okay. I'd rather avoid the dependency but it's maybe no big deal

me too, but since our docs have quite a few deps anyway, and this dependency will be removed when setuptools upgrade their vendored version of packaging, thought it's not big deal.

@adrinjalali adrinjalali merged commit ad57efa into scikit-learn:master Nov 11, 2019
@adrinjalali adrinjalali deleted the sphinx/release branch November 11, 2019 12:46
panpiort8 pushed a commit to panpiort8/scikit-learn that referenced this pull request Mar 3, 2020
…rn#15566)

* use x.y.z (no rc/beta/etc) for version in sphinx config

* use parse_version, and refactor binder branch inference

* install packaging in doc build envs

* use packaging
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0