-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Conversation
Error building the nmslib wheel looking at the logs https://pypi.org/project/nmslib/ And here is the upstream issue: nmslib/nmslib#464 |
Can you please conditionally disable Py3.9 wheel builds on Windows? We'll have to handle them separately when the nsmlib wheels become available. |
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 |
@piskvorky WDYT? Our options for Python 3.9 are:
@FredHappyface Please let me know if the above isn't a correct interpretation of the choice we need to make. |
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 |
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." |
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. |
@FredHappyface Are you able to conditionally disable nmslib-dependent code such that Python3.9 builds for Windows succeed? |
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 |
Thank you for taking this up. Can we handle PyEMD in the same way? I understand that the rabbit hole is getting deeper... |
I'll certainly take a look. And yeah it is I take it then that it's not essential to core functionality? |
Further down the rabbit hole we go... ztane/python-Levenshtein#63 |
Jesus. What kind of apocalypse happened on Windows Py3.9? |
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 |
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 warningPS 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 |
The goodOk managed to fix the bug I introduced to py3.8 testing The badThough the following tests still fail (seems like only on my local machine judging from azure):
EDIT: this isn't reflected in the azure logs so I assume it's some issue with my environment Next stepsHappy 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) |
Think I've sorted this out now. |
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.
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.
gensim/similarities/levenshtein.py
Outdated
try: | ||
import Levenshtein | ||
except ImportError: | ||
raise ImportError("Levenshtein not installed. Please run `pip install Levenshtein`.") |
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.
Careful, the PyPI name of the library we need is https://pypi.org/project/python-Levenshtein/, not Levenshtein
.
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.
Thanks, my bad there! Slight oversight. Will fix
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.
BTW, I don't think we need this try-except here if we catch the import error in gensim.similarities.init.
gensim/test/test_similarities.py
Outdated
@@ -1546,6 +1546,10 @@ def test_inner_product_corpus_corpus_true_true(self): | |||
|
|||
class TestLevenshteinDistance(unittest.TestCase): | |||
def test_max_distance(self): | |||
try: |
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.
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
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 agree, this reads far better
setup.py
Outdated
'testfixtures', | ||
'Morfessor==2.0.2a4', | ||
] | ||
|
||
not_py39_win_testenv = [ |
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 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',
])
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.
Yeah that also makes far more sense. I'll make these changes tomorrow
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? |
Of course! You're on a roll :) But can you merge |
…into py39wheels
Co-authored-by: Michael Penkov <m@penkov.dev>
Merging. Good work @FredHappyface ! Thank you for your contribution. |
Fixes #3056
Follows from #2966
Summary
Added py 3.9 build targets to travis/azure