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

Remove Travis CI #790

Merged
merged 2 commits into from
Sep 11, 2020
Merged

Remove Travis CI #790

merged 2 commits into from
Sep 11, 2020

Conversation

moazreyad
Copy link
Contributor

This PR removes Travis CI since we are using Github Actions now. :octocat:

  • We will not run the cpp and python lint commands:

tool/linting/py.sh
tool/linting/cpp.sh

Instead we can just use the output of Github's lgtm. Is this fine with everyone?

  • We do not have a github workflow that publishes the conda packages. If this is required, we can add the Conda Package Publish Action at the end of the conda workflow. Assuming there is secrets.ANACONDA_PASSWORD and secrets.ANACONDA_USERNAME defined in the github secrets of the user who runs this workflow.

  • Currently the github workflows use the latest Ubuntu only. If it is required to support another older versions, we can add them to the workflow. Travis CI was checking Ubuntu 14.04, 16.04 and 18.04.

@codecov
Copy link

codecov bot commented Sep 6, 2020

Codecov Report

Merging #790 into dev will increase coverage by 0.13%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #790      +/-   ##
==========================================
+ Coverage   65.08%   65.21%   +0.13%     
==========================================
  Files          86      104      +18     
  Lines        4786    12414    +7628     
==========================================
+ Hits         3115     8096    +4981     
- Misses       1671     4318    +2647     
Flag Coverage Δ
#singa-cpp 65.08% <ø> (?)
#singa-python 65.29% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
proto/io_pb2.py 0.00% <0.00%> (ø)
utils.py 65.78% <0.00%> (ø)
data.py 0.00% <0.00%> (ø)
opt.py 48.31% <0.00%> (ø)
proto/core_pb2.py 0.00% <0.00%> (ø)
model.py 89.65% <0.00%> (ø)
sonnx.py 91.94% <0.00%> (ø)
tensor.py 75.19% <0.00%> (ø)
__init__.py 100.00% <0.00%> (ø)
autograd.py 89.99% <0.00%> (ø)
... and 8 more

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 7586a3b...ea17426. Read the comment docs.

@moazreyad moazreyad mentioned this pull request Sep 7, 2020
29 tasks
@chrishkchris
Copy link
Contributor

chrishkchris commented Sep 7, 2020

I have a question: the current CI use the current PR branch for CI test.

In this case, if one PR fixes a bug that cause a failed CI test, some other PR still cannot pass the test unless rebased to dev branch
Also another problem is that if team members A and B working on the same branch, their code can pass the CI test separately, but may not be compatible to each other (e.g. due to API change)

Would it be a better practice if we use the merged code instead of the PR branch for CI test?

@moazreyad
Copy link
Contributor Author

Would it be a better practice if we use the merged code instead of the PR branch for CI test?

It is possible to remove pull_request: from the trigger events in the workflow yaml files:

on:
  push:
  pull_request:

or we can keep it as a guide for the PR reviewer but not requiring all PR to pass all the tests. It will be up to the PR reviewer to decide if the failed tests should be fixed in this PR or not. I would propose to keep it because sometimes it is better to discover the failed tests in the PR before the merge. But all the team can vote on the best options for triggering the workflow. There are many events for workflow trigger and may be some of them can be enabled on some specific workflows.

Also another problem is that if team members A and B working on the same branch, their code can pass the CI test separately, but may not be compatible to each other (e.g. due to API change)

If the push: trigger is enabled, this problem will be discovered when the second team member pushes the code and the workflow is triggered.

@chrishkchris
Copy link
Contributor

chrishkchris commented Sep 10, 2020

I think I got the difference between "push" and "pull_request" clearer now:

From this webpage: https://frontside.com/blog/2020-05-26-github-actions-pull_request/
This page says that:
"This difference means that a pull_request workflow ref would look like refs/remotes/pull/##/merge whereas a push workflow would be refs/heads/branch_name. This explains why the SHA of a push workflow matches the commit that triggered the workflow, whereas the SHA of a pull_request workflow does not; instead the SHA of the pull_request is the resulting commit that was created from merging the base to the head."

So the trigger of pull_request is very useful because it seems to use the merged code (from base to head) for test, which accounts for the compatibility to the base branch.

@nudles
Copy link
Member

nudles commented Sep 11, 2020

This PR removes Travis CI since we are using Github Actions now. :octocat:

* We will not run the cpp and python lint commands:

tool/linting/py.sh
tool/linting/cpp.sh

Instead we can just use the output of Github's lgtm. Is this fine with everyone?

* We do not have a github workflow that publishes the conda packages. If this is required, we can add the [Conda Package Publish Action](https://github.com/marketplace/actions/conda-package-publish-action) at the end of the conda workflow. Assuming there is secrets.ANACONDA_PASSWORD and secrets.ANACONDA_USERNAME defined in the [github secrets](https://docs.github.com/en/actions/configuring-and-managing-workflows/creating-and-storing-encrypted-secrets) of the user who runs this workflow.

How can we set the secrets? There is no setting tab for this project..

* Currently the github workflows use the latest Ubuntu only. If it is required to support another older versions, we can add them to the workflow. Travis CI was checking Ubuntu 14.04, 16.04 and 18.04.

@nudles nudles merged commit 0d79d52 into apache:dev Sep 11, 2020
@moazreyad
Copy link
Contributor Author

How can we set the secrets? There is no setting tab for this project..

There are two solutions:

  1. The secrets can be stored in the forked repository of the project by the person who wants to publish to conda. Then the conda publishing workflow will run only from this forked repository because it is the only place where the secrets are defined.

  2. The better option is to ask the Apache INFRA to add the secrets to the project. In this case we don't store username and password, but we store the conda upload token. You may open a ticket similar to this one and send the token and explicit instructions as to how to create the token to root@apache.org. The same also can be done for publishing to PyPi.

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.

3 participants