Skip to content

Preparing 0.24 release candidate 1. #18956

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 6 commits into from
Dec 2, 2020
Merged

Conversation

cmarmo
Copy link
Contributor

@cmarmo cmarmo commented Dec 2, 2020

What does this implement/fix? Explain your changes.

Update doc and version following the maintainer instructions.

@ogrisel
Copy link
Member

ogrisel commented Dec 2, 2020

We have SCIKIT_LEARN_VERSION=0.24.dev0 hardcoded in .github/workflows/wheels.yml. Ideally this should be made dynamical using:

https://docs.github.com/en/free-pro-team@latest/actions/reference/workflow-commands-for-github-actions

and:

python -c "import sklearn; print(sklearn.__version__)"

@ogrisel
Copy link
Member

ogrisel commented Dec 2, 2020

@alfaro96
Copy link
Member

alfaro96 commented Dec 2, 2020

We have SKLEARN_VERSION=0.24.dev0 hardcoded in .github/workflows/wheels.yml. Ideally this should be made dynamical using:

https://docs.github.com/en/free-pro-team@latest/actions/reference/workflow-commands-for-github-actions

and:

python -c "import sklearn; print(sklearn.__version__)"

IIRC, I tried this solution before and I think that we cannot import scikit-learn since it has not been previously installed.

Nevertheless, I have an alternative solution that would allow to do that dynamically.

PR welcome?

@cmarmo
Copy link
Contributor Author

cmarmo commented Dec 2, 2020

Thanks @ogrisel and @alfaro96 but the Windows wheel is failing with

 Unpacking to: .\scikit_learn-0.24rc1...OK
  + python build_tools/github/vendor.py scikit_learn-0.24.rc1 32
  Traceback (most recent call last):
    File "build_tools/github/vendor.py", line 148, in <module>
      main(wheel_file, bitness)
    File "build_tools/github/vendor.py", line 110, in main
      raise RuntimeError(f"Could not find {wheel_dirname} file.")
  RuntimeError: Could not find scikit_learn-0.24.rc1 file.

Do you mind letting me some time to debug? Thanks!

@ogrisel
Copy link
Member

ogrisel commented Dec 2, 2020

IIRC, I tried this solution before and I think that we cannot import scikit-learn since it has not been previously installed.

Indeed. It's possible but it's hackish to do it this way:

$ python -c "import builtins; builtins.__SKLEARN_SETUP__=True; import sklearn; print(sklearn.__version__)"
Partial import of sklearn during the build process.
0.24.dev0

Alternatively we could open sklearn/__init__.py and use a find the line with __version__ definition and eval only this line.

+1 for doing this as a PR on master.

@alfaro96
Copy link
Member

alfaro96 commented Dec 2, 2020

IIRC, I tried this solution before and I think that we cannot import scikit-learn since it has not been previously installed.

Indeed. It's possible but it's hackish to do it this way:

$ python -c "import builtins; builtins.__SKLEARN_SETUP__=True; import sklearn; print(sklearn.__version__)"
Partial import of sklearn during the build process.
0.24.dev0

Alternatively we could open sklearn/__init__.py and use a find the line with __version__ definition and eval only this line.

+1 for doing this as a PR on master.

My solution is just to use WHEEL_DIRNAME=$(ls -d scikit-learn-*) since we know that this is the unpack directory (regardless of the scikit-learn version). I do not know why I did not use this solution in first place.

I will open a PR in a few minutes.

@ogrisel
Copy link
Member

ogrisel commented Dec 2, 2020

@cmarmo the fix in master has been merged: #18957. do you want to cherry-pick it to your PR? The squashed commit is 671069c.

@cmarmo
Copy link
Contributor Author

cmarmo commented Dec 2, 2020

@cmarmo the fix in master has been merged: #18957. do you want to cherry-pick it to your PR? The squashed commit is 671069c.

Sorry @ogrisel, second thought... don't you think it would be better to have this merged in the 0.24.X branch?

...
never mind

@cmarmo
Copy link
Contributor Author

cmarmo commented Dec 2, 2020

Thanks @ogrisel and @alfaro96 but the Windows wheel is failing with

 Unpacking to: .\scikit_learn-0.24rc1...OK
  + python build_tools/github/vendor.py scikit_learn-0.24.rc1 32
  Traceback (most recent call last):
    File "build_tools/github/vendor.py", line 148, in <module>
      main(wheel_file, bitness)
    File "build_tools/github/vendor.py", line 110, in main
      raise RuntimeError(f"Could not find {wheel_dirname} file.")
  RuntimeError: Could not find scikit_learn-0.24.rc1 file.

Do you mind letting me some time to debug? Thanks!

I wrote differently the sklearn version in wheel.yml and __init__.py that's why it was failing before and it works with the dynamic directory naming.

@ogrisel
Copy link
Member

ogrisel commented Dec 2, 2020

I have opened a sister PR to master to update the version number: #18958.

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM. Let's merge.

@ogrisel ogrisel merged commit 0c41188 into scikit-learn:0.24.X Dec 2, 2020
@cmarmo cmarmo deleted the 0.24.X-patch1 branch December 2, 2020 17:39
@ogrisel
Copy link
Member

ogrisel commented Dec 2, 2020

The test pypi upload was successful: https://test.pypi.org/project/scikit-learn/0.24.0rc1/#files

@ogrisel
Copy link
Member

ogrisel commented Dec 2, 2020

Apparently there is a way to package release candidates on conda-forge but it involves manual operations as described in:

https://github.com/conda-forge/cfep/blob/master/cfep-05.md

However I am not sure how we are supposed to create this recipe/conda_build_config.yaml file. We just have recipe/meta.yml in the feedstock at the moment.

Maybe we can skip conda-forge packaging for the RC.

@alfaro96
Copy link
Member

alfaro96 commented Dec 3, 2020

Maybe you could use a similar approach than the pandas-feedstock: https://github.com/conda-forge/pandas-feedstock/pull/84/files 🤔.

@ogrisel
Copy link
Member

ogrisel commented Dec 3, 2020

I gave it a try here:

conda-forge/scikit-learn-feedstock#138

@ogrisel
Copy link
Member

ogrisel commented Dec 3, 2020

The 0.24 documentation is now online:

https://scikit-learn.org/0.24/

It's a good time to review to spot possible problems it before we make the release-0.24.0 final PR.

@cmarmo
Copy link
Contributor Author

cmarmo commented Dec 7, 2020

The 0.24 documentation is now online:

https://scikit-learn.org/0.24/

It's a good time to review to spot possible problems it before we make the release-0.24.0 final PR.

Perhaps someone could merge #18899? Easy and already approved simplification of the current css.

@stefan-it
Copy link

stefan-it commented Dec 7, 2020

Hi guys,

Flair team here 😅

When using the current 0.24.0rc1 version, our builds are currently failing with:

Partial import of sklearn during the build process.
Traceback (most recent call last):
  File "/tmp/easy_install-s6byaeqi/scikit-learn-0.24.0rc1/setup.py", line 201, in check_package_status
    module = importlib.import_module(package)
  File "/opt/hostedtoolcache/Python/3.6.12/x64/lib/python3.6/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 994, in _gcd_import
  File "<frozen importlib._bootstrap>", line 971, in _find_and_load
  File "<frozen importlib._bootstrap>", line 953, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'numpy'
Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.6.12/x64/lib/python3.6/site-packages/setuptools/sandbox.py", line 154, in save_modules
    yield saved
  File "/opt/hostedtoolcache/Python/3.6.12/x64/lib/python3.6/site-packages/setuptools/sandbox.py", line 195, in setup_context
    yield
  File "/opt/hostedtoolcache/Python/3.6.12/x64/lib/python3.6/site-packages/setuptools/sandbox.py", line 250, in run_setup
    _execfile(setup_script, ns)
  File "/opt/hostedtoolcache/Python/3.6.12/x64/lib/python3.6/site-packages/setuptools/sandbox.py", line 45, in _execfile
    exec(code, globals, locals)
  File "/tmp/easy_install-s6byaeqi/scikit-learn-0.24.0rc1/setup.py", line 305, in <module>
  File "/tmp/easy_install-s6byaeqi/scikit-learn-0.24.0rc1/setup.py", line 291, in setup_package
  File "/tmp/easy_install-s6byaeqi/scikit-learn-0.24.0rc1/setup.py", line 227, in check_package_status
ImportError: numpy is not installed.
scikit-learn requires numpy >= 1.13.3.
Installation instructions are available on the scikit-learn website: http://scikit-learn.org/stable/install.html

See build log here. Our requirements file is specified here.

Do we need to explict. specify numpy in our requirements file when we want to use the upcoming 0.24 release 🤔

Thanks many in advance 🤗

@cmarmo
Copy link
Contributor Author

cmarmo commented Dec 7, 2020

Hi @stefan-it, thanks for your feedback. I've reproduced the error locally ... almost: my install fails installing numpy as scikit-learn dependence ... The problem is that building with python 3.6 the last version of numpy complains with

  File "/tmp/easy_install-5rt1jezv/numpy-1.20.0rc1/setup.py", line 30, in <module>
RuntimeError: Python version >= 3.7 required.

Probably @ogrisel or @alfaro96 know better what needs to be fixed here...

@alfaro96
Copy link
Member

alfaro96 commented Dec 7, 2020

Hi @stefan-it, thanks for your feedback. I've reproduced the error locally ... almost: my install fails installing numpy as scikit-learn dependence ... The problem is that building with python 3.6 the last version of numpy complains with

  File "/tmp/easy_install-5rt1jezv/numpy-1.20.0rc1/setup.py", line 30, in <module>
RuntimeError: Python version >= 3.7 required.

Can you reproduce the error locally using numpy<1.20.0?

Probably @ogrisel or @alfaro96 know better what needs to be fixed here...

Python 3.6 is no longer supported by numpy>=1.20.0. See #18900 (comment).

@cmarmo
Copy link
Contributor Author

cmarmo commented Dec 7, 2020

Can you reproduce the error locally using numpy<1.20.0

If I force numpy<1.20.0 in the requirements.txt the build goes well: if I just add numpy in the requirements the build fails with the same error as above.
In a python3.7 environment the build goes well without adding requirements.
I suppose some more details about dependencies should be added on the flair side?
@stefan-it do you have a way to test if your CI works for python 3.7? Thanks a lot!

@alfaro96
Copy link
Member

alfaro96 commented Dec 7, 2020

Can you reproduce the error locally using numpy<1.20.0

If I force numpy<1.20.0 in the requirements.txt the build goes well: if I just add numpy in the requirements the build fails with the same error as above.

That makes sense because Python 3.6 is no longer supported by numpy.

Thank you @cmarmo for testing this :)

@stefan-it
Copy link

stefan-it commented Dec 7, 2020

Thanks for your help ❤️

I can confirm that with Python 3.6, the build error can be fixed by using numpy<1.20.0 in our requirements.txt 🤗

With Python 3.7 it is not necessary to specify numpy<1.20.0 (and it would also work with numpy<1.20.0) - I just tested it in a clean environment.

As we have a large user base that is using Python 3.6, we'll use numpyversion restriction in requirements.txt for now :)

@stefan-it
Copy link

stefan-it commented Dec 7, 2020

Update: I was using a local environment with python:3.6-buster to test the numpy version restriction for numpy<1.20.0. This was working.

However, with Ubuntu 18.04 (that GitHub Actions is using), the build will fail again - I was also able to reproduce it with an ubuntu:18.04 container. Our latest build log is here.

Do you have any idea, why the numpy version is not recognized? The build log expl. mentions that "numpy 1.19.4" was installed 🤔

Many thanks!

@ogrisel
Copy link
Member

ogrisel commented Dec 8, 2020

Thanks for the investigation, we can probably try to add a conditional constraint to numpy<1.20.0 for Python 3.6 in our runtime dependency declaration in our setup.py file (although I am not 100% sure of the possibility to declare such conditional runtime deps).

@ogrisel
Copy link
Member

ogrisel commented Dec 8, 2020

@stefan-it in your case I do not understand why your CI build does not install the scikit-learn wheel. Here it seems to be trying to install run the setup.py of scikit-learn from source (probably to introspect the dependencies of gdown).

Which command is run by your CI to get this log?

@ogrisel
Copy link
Member

ogrisel commented Dec 8, 2020

Ok I get it: your project dependencies are set using:

https://github.com/flairNLP/flair/blob/master/setup.py#L3

and then you run python setup.py develop -q to install the dependencies which is using setuptools / easy install.

I think you should use pip explicitly to install everything, either:

pip install -r requirements.txt
python setup.py develop -q

or:

pip install --editable .

@ogrisel
Copy link
Member

ogrisel commented Dec 8, 2020

But this reveals that scikit-learn setup.py might still have a problem: we shouldn't need to import numpy just to compute the scikit-learn deps with setup.py.

@stefan-it
Copy link

Hi @ogrisel ,

thanks for your help! We decided to use the pip install -r requirements.txt && python setup.py develop -q and all builds are working now ❤️

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.

4 participants