-
-
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
Move x86 tests from Travis to GHA, add aarch64 wheel build to Travis #3026
Conversation
@mpenkov I moved the x86 tests to GHA to preserve Travis credits. |
.github/workflows/tests.yml
Outdated
pull_request: | ||
branches: [ develop ] | ||
schedule: | ||
- cron: '0 0 * * sun,wed' |
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.
Why is this here? These should run on each PR, not on a schedule.
.github/workflows/tests.yml
Outdated
- name: Enable core dumps | ||
run: ulimit -c unlimited -S # enable core dumps | ||
- name: Run tox tests | ||
run: tox -e ${{ matrix.tox }} -vv |
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.
Is vv necessary here? Tox will spew lots of output that nobody really ever reads.
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'm following the existing tests in .travis.yml. I can remove it.
.travis.yml
Outdated
dist: trusty | ||
branches: | ||
only: | ||
- develop |
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.
What does this file actually do? It looks to me like it will build a wheel on every commit, which isn't what we want, right?
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 guess not. I can build on every release. Is that done through master?
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.
Releases are done on each tag that matches the regex v\d+\.\d+\.\d+
.
But we also want to build wheels twice a week (like with the other github workflow that you implemented).
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.
You can create a cron job from the UI. Can't find any documentation about setting it up through the .travis.yml file. https://docs.travis-ci.com/user/cron-jobs/
.travis.yml
Outdated
- build_wheel $REPO_DIR $PLAT | ||
script: | ||
- install_run $PLAT | ||
after_success: |
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.
Why are after_success and after_failure steps exactly the same?
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.
Should I remove the after_failure? I followed what gensim-wheels was doing.
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.
If the two are identical, then yes. More specifically:
- remove after_failure
- make after_success another step (the last step in the workflow)
- ensure that all steps in the workflow run, even if preceding steps fail
@mpenkov I've made the changes. |
@janaknat Thank you. In the future, can you please avoid force-pushing? It makes it more difficult to review your PR as a maintainer. |
@mpenkov I've made the changes. |
@@ -1,6 +1,6 @@ | |||
branches: | |||
only: | |||
- master | |||
- /v\d+\.\d+\.\d+/ |
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 needs to match to a tag name, not a branch name.
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.
Based on https://docs.travis-ci.com/user/customizing-the-build/#safelisting-or-blocklisting-branches I believe having a regex in branches.only will only build tagged commits.
Tagged commit build: https://travis-ci.com/github/janaknat/gensim/builds/214207365
@mpenkov Is there anything in the PR that I should address? |
@mpenkov Ping. |
Sorry, I was busy with other projects during the week. Thank you for your patience. Let's merge this and see how it plays out in the wild. |
Looks like there are some problems, @janaknat please investigate. https://github.com/RaRe-Technologies/gensim/runs/1796402952?check_suite_focus=true |
@mpenkov Are there any caches being used? The errors look like issues with trying to download tox, gdb and the IP not being accessible. |
Not that I'm aware of. The github actions stuff is all very new, from memory, you're one of the first people to contribute to it with gensim. |
@mpenkov Any dates on when the aarch64 wheels show up in PyPI? |
For my part, I plan to do an open source sprint in February to finish up my tasks for the 4.0.0 Milestone. Which of course doesn't mean we cannot do another beta / alpha release before that – up to @mpenkov . |
@piskvorky @mpenkov Any updates on when the aarch64 wheels will get released? |
We're planning to release 4.0.0rc1 (release candidate) this week. And then the "full" 4.0.0 right after :) |
That's great. aarch64 is on track to get wheels this time around? |
Hm, I thought that's what this PR was about. @mpenkov will any other steps be necessary to get from here to wheels on PyPI? |
We'll look at it as part of the release process. If there's any additional work required to get the wheels into PyPI once they're built, we'll take care of it then. |
Hey @mpenkov @piskvorky I see gensim has a new release. I don't see aarch64 wheels. Any issues? |
I couldn't get the pipeline working when making the release. I'll debug the issue. Thank you for reminding me. |
@janaknat Please have a look at this: https://travis-ci.org/github/RaRe-Technologies/gensim/jobs/764640506 |
@mpenkov You need to migrate to Travis-ci.com from travis-ci.org. Arm64 runners are only available in .com. |
OK, so this is related to #3083. |
Move Travis x86 tests to Github Actions. Also, add Travis-CI config to build aarch64 wheels.