Skip to content
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

Add py39 wheels to travis/azure #3058

Merged
merged 10 commits into from
Mar 15, 2021
Merged

Add py39 wheels to travis/azure #3058

merged 10 commits into from
Mar 15, 2021

Conversation

FredHappyface
Copy link
Contributor

Fixes #3056
Follows from #2966

Summary

Added py 3.9 build targets to travis/azure

@FredHappyface
Copy link
Contributor Author

FredHappyface commented Mar 2, 2021

Error building the nmslib wheel looking at the logs https://pypi.org/project/nmslib/

And here is the upstream issue: nmslib/nmslib#464

@mpenkov
Copy link
Collaborator

mpenkov commented Mar 4, 2021

Can you please conditionally disable Py3.9 wheel builds on Windows?

We'll have to handle them separately when the nsmlib wheels become available.

@FredHappyface
Copy link
Contributor Author

FredHappyface commented Mar 4, 2021

Had a go at that but broken things. looks to me like it's not really supported?

microsoft/azure-pipelines-agent#1501

This basically describes what's happened here https://developercommunity.visualstudio.com/t/how-can-i-skipmake-condition-matrix-jobs-in-yaml/757154

Is it wroth just removing py39-win for now?

@mpenkov
Copy link
Collaborator

mpenkov commented Mar 5, 2021

@piskvorky WDYT?

Our options for Python 3.9 are:

  1. Build wheels for non-Windows builds
  2. Build nothing until nmslib wheels are available

@FredHappyface Please let me know if the above isn't a correct interpretation of the choice we need to make.

@FredHappyface
Copy link
Contributor Author

I would find it useful if we went ahead and built Windows wheels anyway as that is used in a pretty small component if I remember correctly?

But then again I would understand why you might be reluctant to do so. Of those two options I'd be most inclined to go with the former

@gojomo
Copy link
Collaborator

gojomo commented Mar 5, 2021

Indeed if only one narrow function doesn't work, due to some other library not working, it could make sense to conditionally disable the test, make that a new open issue, and add a release-note that says: "NMS-related features don't work under Python 3.9 on Windows due to a problem with the [whatever] project, use an lower Python version if you need those functions."

@piskvorky
Copy link
Owner

piskvorky commented Mar 5, 2021

Yes. We definitely don't want to drop Windows builds on account of a marginal (in the sense of barely used in Gensim) external library.

Even dropping NMS from Gensim altogether would be a better resolution. Gensim Windows users >>> Gensim NMS users = ~zero.

But if possible, disabling only affected tests & waiting until NMS is fixed preferable.

@mpenkov
Copy link
Collaborator

mpenkov commented Mar 5, 2021

@FredHappyface Are you able to conditionally disable nmslib-dependent code such that Python3.9 builds for Windows succeed?

@FredHappyface
Copy link
Contributor Author

FredHappyface commented Mar 5, 2021

Manged to do that but now stymied by pyemd. Not sure if you want me to push what I've got or hold off for a bit?

And here is the upstream issue wmayner/pyemd#48

@mpenkov
Copy link
Collaborator

mpenkov commented Mar 6, 2021

Thank you for taking this up. Can we handle PyEMD in the same way? I understand that the rabbit hole is getting deeper...

@FredHappyface
Copy link
Contributor Author

I'll certainly take a look. And yeah it is

I take it then that it's not essential to core functionality?

@FredHappyface
Copy link
Contributor Author

FredHappyface commented Mar 6, 2021

Further down the rabbit hole we go... ztane/python-Levenshtein#63

@piskvorky
Copy link
Owner

piskvorky commented Mar 6, 2021

Jesus. What kind of apocalypse happened on Windows Py3.9?

@FredHappyface
Copy link
Contributor Author

FredHappyface commented Mar 6, 2021

No idea - a few things have been deprecated but not sure why it's destroyed so many extensions

Someone has made a fork with windows py39 wheels ztane/python-Levenshtein#61

@FredHappyface
Copy link
Contributor Author

FredHappyface commented Mar 6, 2021

Looks like we are getting the same error as in the upstream repos on my local machine. (I am certain that MS BuildTools is installed). I have no idea what is causing this

Spoiler warning
PS C:\Users\Dell\Documents\GitHub\gensim> tox -e py39-win
py39-win create: C:\Users\Dell\Documents\GitHub\gensim\.tox\py39-win
py39-win installdeps: pip>=19.1.1, .[test-win]
py39-win installed: atomicwrites==1.4.0,attrs==20.3.0,colorama==0.4.4,Cython==0.29.22,gensim @ file:///C:/Users/Dell/Documents/GitHub/gensim,iniconfig==1.1.1,mock==4.0.3,Morfessor==2.0.2a4,numpy==1.20.1,packaging==20.9,pluggy==0.13.1,py==1.10.0,pyparsing==2.4.7,pytest==6.2.2,scipy==1.6.1,smart-open==4.2.0,testfixtures==6.17.1,toml==0.10.2
py39-win run-test-pre: PYTHONHASHSEED='1'        
py39-win run-test: commands[0] | python --version
Python 3.9.2
py39-win run-test: commands[1] | pip --version
pip 21.0.1 from C:\Users\Dell\Documents\GitHub\gensim\.tox\py39-win\lib\site-packages\pip (python 3.9)
py39-win run-test: commands[2] | python setup.py build_ext --inplace
C:\Users\Dell\Documents\GitHub\gensim\.tox\py39-win\lib\site-packages\setuptools\dist.py:461: UserWarning: Normalizing '4.0.0beta' to '4.0.0b0'
warnings.warn(tmpl.format(**locals()))
running build_ext
building 'gensim.models.word2vec_inner' extension
error: Microsoft Visual C++ 14.0 or greater is required. Get it with "Microsoft C++ Build Tools": https://visualstudio.microsoft.com/visual-cpp-build-tools/
ERROR: InvocationError for command 'C:\Users\Dell\Documents\GitHub\gensim\.tox\py39-win\Scripts\python.EXE' setup.py build_ext --inplace (exited with code 1)_________________________________________________________________________ summary __________________________________________________________________________
ERROR:   py39-win: commands failed

Running python setup.py build_ext --inplace outside of tox works fine though

@FredHappyface
Copy link
Contributor Author

FredHappyface commented Mar 6, 2021

The good

Ok managed to fix the bug I introduced to py3.8 testing
And managed to skip tests relying on Levenshtein for py3.9

The bad

Though the following tests still fail (seems like only on my local machine judging from azure):

FAILED gensim/test/test_corpora.py::TestMmCorpusWithIndex::test_indexing - ValueError: unable to parse line: b'\r\n'
FAILED gensim/test/test_corpora.py::TestMmCorpusWithIndex::test_load - ValueError: unable to parse line: b' 1.0\r\n'
FAILED gensim/test/test_corpora.py::TestMmCorpusNoIndex::test_indexing - ValueError: unable to parse line: b'\r\n'
FAILED gensim/test/test_corpora.py::TestMmCorpusNoIndexGzip::test_indexing - ValueError: unable to parse line: b'\r\n'
FAILED gensim/test/test_corpora.py::TestMmCorpusNoIndexBzip::test_indexing - ValueError: unable to parse line: b'\r\n'
FAILED gensim/test/test_corpora.py::TestMmCorpusCorrupt::test_indexing - ValueError: unable to parse line: b'\r\n'
FAILED gensim/test/test_corpora.py::TestMmCorpusOverflow::test_indexing - ValueError: unable to parse line: b'\r\n'
FAILED gensim/test/test_corpora.py::TestSvmLightCorpus::test_indexing - TypeError: 'NoneType' object is not subscriptable
FAILED gensim/test/test_corpora.py::TestBleiCorpus::test_indexing - IndexError: list index out of range
FAILED gensim/test/test_corpora.py::TestLowCorpus::test_indexing - AssertionError: Lists differ: [(0, 1), (3, 1), (4, 1)] != []   
FAILED gensim/test/test_corpora.py::TestUciCorpus::test_indexing - ValueError: unable to parse line: b' \r\n'
FAILED gensim/test/test_corpora.py::TestMalletCorpus::test_indexing - IndexError: list index out of range
FAILED gensim/test/test_ldamodel.py::TestLdaModel::test_get_document_topics - ValueError: unable to parse line: b'1.0\r\n'        
FAILED gensim/test/test_ldamodel.py::TestLdaMulticore::test_get_document_topics - ValueError: unable to parse line: b'1.0\r\n'

EDIT: this isn't reflected in the azure logs so I assume it's some issue with my environment

Next steps

Happy for me to push this? (and is there anyway to disable the tests as I'm assuming these pushes are beginning to use up your azure hours)

@FredHappyface
Copy link
Contributor Author

Think I've sorted this out now.

Copy link
Collaborator

@mpenkov mpenkov left a comment

Choose a reason for hiding this comment

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

Thank you for your effort and persistence in getting this to work.

I left you some minor comments. Please have a look and get back to me.

try:
import Levenshtein
except ImportError:
raise ImportError("Levenshtein not installed. Please run `pip install Levenshtein`.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Careful, the PyPI name of the library we need is https://pypi.org/project/python-Levenshtein/, not Levenshtein.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, my bad there! Slight oversight. Will fix

Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW, I don't think we need this try-except here if we catch the import error in gensim.similarities.init.

@@ -1546,6 +1546,10 @@ def test_inner_product_corpus_corpus_true_true(self):

class TestLevenshteinDistance(unittest.TestCase):
def test_max_distance(self):
try:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a bit messy. You shouldn't be testing for dependencies in so many separate places (once in the actual source code, and multiple times in tests). I think there's a better way to do it.

First, in gensim/similarities/__init__.py:

import warning
try:
    import Levenshtein
except ImportError:
    warning.warn(
        "The gensim.similarities.levenshtein submodule is disabled, because the optional "
        "Levenshtein package <https://pypi.org/project/python-Levenshtein/> is unavailable. "
        "Install Levenhstein (e.g. `pip install Levenshtein`) to suppress this warning."
    )
    LevenshteinSimilarityIndex = None
else:
    from .levenshtein import LevenshteinSimilarityIndex

Then in your tests:

import gensim.similarities

@unittest.skipIf(gensim.similarities.LevenshteinSimilarityIndex is None, "gensim.similarities.levenshtein is disabled")
def test_method(...):
    pass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, this reads far better

setup.py Outdated
'testfixtures',
'Morfessor==2.0.2a4',
]

not_py39_win_testenv = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is slightly more complicated than it really needs to be. You want to avoid installing these packages for Python 3.9 under Windows, and install them everywhere else. So why not do something like:

if not (sys.platform.lower().startswith("win") and sys.version[:2] >= (3, 9)):
    core_testenv.extend([
        'pyemd',
        'nmslib',
        'python-Levenshtein >= 0.10.2',
    ])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that also makes far more sense. I'll make these changes tomorrow

@FredHappyface
Copy link
Contributor Author

I've noticed loads of warnings in Azure that I'm confident can be fixed reasonably quickly, so I've created an issue here #3066. Would you be happy for me to take a look at this?

@piskvorky
Copy link
Owner

piskvorky commented Mar 9, 2021

Of course! You're on a roll :)

But can you merge develop first? There were some changes recently, like removing some of the dependencies such as viz. To avoid wasted effort & potential git conflicts. Thanks.

Co-authored-by: Michael Penkov <m@penkov.dev>
@mpenkov mpenkov merged commit aa92604 into piskvorky:develop Mar 15, 2021
@mpenkov
Copy link
Collaborator

mpenkov commented Mar 15, 2021

Merging. Good work @FredHappyface ! Thank you for your contribution.

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.

Provide Python3.9 Wheels
4 participants