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 Github Actions x86 and mac jobs to build python wheels #3024

Merged
merged 5 commits into from
Jan 15, 2021
Merged

Add Github Actions x86 and mac jobs to build python wheels #3024

merged 5 commits into from
Jan 15, 2021

Conversation

janaknat
Copy link
Contributor

multibuild added as a submodule. It is used for the building and testing process.

@janaknat
Copy link
Contributor Author

PLAT: [x86_64]
env:
PKG_NAME: gensim
BUILD_COMMIT: 4.0.0beta
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 it should build the latest HEAD here. There's no point building the same commit every two weeks, is there?

BUILD_COMMIT: 4.0.0beta
UNICODE_WIDTH: 32
MB_PYTHON_VERSION: ${{ matrix.python-version }}
TRAVIS_PYTHON_VERSION: ${{ matrix.python-version }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need separate TRAVIS_ environment variables here? We aren't using Travis anymore.

Some of these values are redundant (e.g. MB_PYTHON_VERSION and TRAVIS_PYTHON_VERSION). Some of them are already available via the default environment variables exposed by Github Actions. Let's only keep the ones that we really need.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(if multibuild expects those variables to be there, it's definitely worth documenting that)

uses: actions/setup-python@v2
with:
python-version: ${{ matrix.python-version }}
- name: Pin Morfessor, Levenshtein, visdom
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need a separate step to pin these things. It's simpler to define them in the environment explicitly, right?

e.g.

env:
   TEST_DEPENDS: Morfessor==2.0.2a4 ...
```

else
echo "TRAVIS_OS_NAME=${{ matrix.os }}" >> $GITHUB_ENV;
fi
if [ "${{ matrix.python-version }} != 3.6" && "ubuntu-latest == ${{ matrix.os }}" ]; then
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 you're over-depending on bash conditionals here. You're mixing actual build steps and configuration, so it's difficult to understand what's going on without reading through the entire workflow.

Isn't it possible to define the environment variables as part of the matrix? For example:

source config.sh
echo "------ BEFORE INSTALL ---------"
before_install
#echo "------ CLEAN CODE --------"
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this for? If we don't need it, please remove it.

echo "MF_TEST_DEP=$(echo Morfessor==2.0.2a4)" >> $GITHUB_ENV;
echo "LV_TEST_DEP=$(echo python-levenshtein==0.12.0)" >> $GITHUB_ENV;
echo "VD_TEST_DEP=$(echo visdom==0.1.8.9)" >> $GITHUB_ENV
- name: Setup Environment variables
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 most of this step needs to go into the environment sub-sections of the build matrix.

@janaknat
Copy link
Contributor Author

@mpenkov I've updated the branch and made the changes.


on:
push:
branches: [ master ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
branches: [ master ]
branches: [ develop ]

Same below.

We use master for releases. PRs get merged onto develop, so we should be building from there.

with:
submodules: recursive
fetch-depth: 0
- name: Setup OSX var for Multibuild
Copy link
Collaborator

Choose a reason for hiding this comment

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

if: ${{ matrix.os == 'macos-latest' }}
run: |
echo "TRAVIS_OS_NAME=osx" >> $GITHUB_ENV;
- name: Setup SKIP_NETWORK_TEST env variable
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above.

Having these variables defined in the matrix is better, because then we can see what's going on up front, instead of having to read the build script step by step.

source multibuild/common_utils.sh
source multibuild/travis_steps.sh
source config.sh
echo "------ BEFORE INSTALL ---------"
Copy link
Collaborator

@mpenkov mpenkov Jan 14, 2021

Choose a reason for hiding this comment

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

There's a neat way to group log output in github actions:

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

So we can do things like:

run: |
  echo ::group::Set up multibuild
  source multibuild/common_utils.sh
  source multibuild/travis_steps.sh
  source config.sh
  echo ::endgroup::

  echo ::group::before_install
  before_install
  echo ::endgroup::

and so on.

@mpenkov
Copy link
Collaborator

mpenkov commented Jan 14, 2021

OK, that's looking better. Left you some more comments.

It uses multibuild for the building and testing process.
@janaknat
Copy link
Contributor Author

@mpenkov Addressed the comments.

@mpenkov mpenkov merged commit 959f2dd into piskvorky:develop Jan 15, 2021
@mpenkov
Copy link
Collaborator

mpenkov commented Jan 15, 2021

@janaknat I made some minor edits and merged your changes. Thank you for your contribution!

Please keep an eye on the new workflow - if there's anything else you need to add, please let me know via a PR. Thanks again!

@mpenkov
Copy link
Collaborator

mpenkov commented Jan 17, 2021

@janaknat The wheels are getting built for each commit - this isn't what we wanted, right? They should run twice weekly and that's it.

https://github.com/RaRe-Technologies/gensim/actions/runs/491235040

Please have a look.

@janaknat
Copy link
Contributor Author

@mpenkov Any pushes, PRs to develop will trigger a build, in addition to running twice a week.

@mpenkov
Copy link
Collaborator

mpenkov commented Jan 17, 2021

Can you change that behavior? Building wheels on every push and PR is excessive. We only need them built once a week.

@janaknat
Copy link
Contributor Author

Ok, I'll create a PR and change it to run twice a week.

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.

2 participants