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

test and build wheels for Py3.{7,8,9,10} #3298

Merged
merged 50 commits into from
Mar 18, 2022
Merged

test and build wheels for Py3.{7,8,9,10} #3298

merged 50 commits into from
Mar 18, 2022

Conversation

mpenkov
Copy link
Collaborator

@mpenkov mpenkov commented Feb 26, 2022

No description provided.

@mpenkov mpenkov added this to the Next release milestone Feb 26, 2022
@codecov
Copy link

codecov bot commented Feb 26, 2022

Codecov Report

Merging #3298 (a3c909f) into develop (86b1832) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #3298   +/-   ##
========================================
  Coverage    79.53%   79.53%           
========================================
  Files           68       68           
  Lines        11781    11781           
========================================
  Hits          9370     9370           
  Misses        2411     2411           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86b1832...a3c909f. Read the comment docs.

@mpenkov
Copy link
Collaborator Author

mpenkov commented Mar 12, 2022

Oof, finally got everything to build for Python 3.10.

There are some catches:

  • Had to jump through hoops to upgrade pip for Py3.10, so there are some hacky scripts
  • Gave up on running unit tests using the wheel, because some blinking tests can fail the build. I'm using a much simpler test now: install the wheel, import gensim, print its version.

Please let me know what you think.

@piskvorky
Copy link
Owner

piskvorky commented Mar 12, 2022

The simpler the better :)

You know way more about this subsystem than I do @mpenkov, so if you say it works (~works better), let's merge.

@mpenkov
Copy link
Collaborator Author

mpenkov commented Mar 18, 2022

OK, I re-enabled the tests and increased the tolerance for the test that was failing. It works, so I'm merging this now.

@mpenkov mpenkov merged commit acce8a2 into develop Mar 18, 2022
@mpenkov mpenkov deleted the fix-ci branch March 18, 2022 14:05
@mpenkov mpenkov mentioned this pull request Mar 18, 2022
env:
PKG_NAME: gensim
REPO_DIR: gensim
BUILD_COMMIT: HEAD
PLAT: x86_64
UNICODE_WIDTH: 32
MB_PYTHON_VERSION: ${{ matrix.python-version }} # MB_PYTHON_VERSION is needed by Multibuild
TEST_DEPENDS: Morfessor==2.0.2a4 python-levenshtein==0.12.0 visdom==0.1.8.9 pytest pytest-cov mock cython nmslib pyemd testfixtures scikit-learn pyemd
TEST_DEPENDS: pytest mock testfixtures
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will removing pyemd (& perhaps others, as well) mean that certain tests we want run are skipped because of missing-libraries?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's possible, but this is for the purpose of testing the wheel, so running tests that require those dependencies isn't strictly necessary.

import urllib.request
with tempfile.NamedTemporaryFile(suffix='.py') as fout:
urllib.request.urlretrieve("https://bootstrap.pypa.io/get-pip.py", fout.name)
subprocess.call([sys.executable, fout.name])
Copy link
Collaborator

@gojomo gojomo Mar 14, 2022

Choose a reason for hiding this comment

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

From a security-in-depth perspective, fetching & immediately running unversioned raw code off the web spooks me, & I'd avoid it if at all possible. Sure, this appears to be from the right pypa.io domain, & thus reflecting the software of a team/project (Python Packaging Authority) which we're already implicitly trusting via things like pip/PyPI installs that the same people/processes control.

But I know the PyPI repository is both a vulnerability point, & a major focus of attackers, and thus also a place where (I hope & expect) code-provenance & system-security matters are taken very seriously. Maybe pypa.io is as rigorous – they should be! – but the naked https://bootstrap.pypa.io/ directory isn't quite offering the signals-of-rigor I'd most like to see.

Maybe the added risk is considered acceptable, maybe we have to use this as a hacky workaround for a while – but we should try to retire it the moment it's not necessary, and we could take extra steps to ensure we're getting a consistent version of get-pip.py. For example: we could grab it from the github project by version-hash at https://github.com/pypa/get-pip/blob/main/public/get-pip.py, to be sure we only get a new one when we choose to roll forward – so our builds always use the exact version we verified OK during testing of this PR, nothing later (either improved or compromised). If this in fact misses a useful true update, causing something to break someday – maybe that's OK, because that's enough time passed to double-check if this kludge is still required.

@gojomo
Copy link
Collaborator

gojomo commented Mar 18, 2022

Sorry, I thought my 2 code review comments had been posted days ago – they were visible to me, even outside the 'changed files' page – but the pending notation alongside them, that I thought meant you hadn't yet dismissed, actually meant I hadn't yet submitted the batch for wider visibility. Ugh. I should probably stick to "submit as individual comment".

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.

None yet

3 participants