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

setup GitHub Actions #243

Merged
merged 14 commits into from
Jul 20, 2020
Merged

setup GitHub Actions #243

merged 14 commits into from
Jul 20, 2020

Conversation

StrikerRUS
Copy link
Member

Hmm, seems that Actions can be triggered even without config in the master branch:

image

One more step is to make passing tests at GitHub Actions for PRs required. Refer to microsoft/LightGBM#3119 (comment)

run: |
if [[ "${{ matrix.lang }}" == "API" ]]; then
export TEST="API";
export COVERALLS_REPO_TOKEN="COVERALLS TOKEN FOR M2CGEN";
Copy link
Member Author

Choose a reason for hiding this comment

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

FIXME

Copy link
Member

Choose a reason for hiding this comment

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

I'm not quite sure how to overcome this. It doesn't seem to be a good idea to expose the API token and make it a part of the publicly available source code. Or am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

Shall we use a Coveralls action (https://github.com/marketplace/actions/coveralls-github-action) instead of manually invoking the coveralls binary in the test.sh script?

Copy link
Member Author

@StrikerRUS StrikerRUS Jul 16, 2020

Choose a reason for hiding this comment

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

API tokens are exposed in a secure way via encoded values.

Very good idea to migrate to Coveralls action!

Anyway, for this PR I believe it's better simply disable Coveralls command at GitHub Actions. Done in 12147b7.

Comment on lines +44 to +46
export LC_ALL="en_US.UTF-8";
sudo locale-gen $LC_ALL;
sudo update-locale;
Copy link
Member Author

@StrikerRUS StrikerRUS Jun 12, 2020

Choose a reason for hiding this comment

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

Workaround for

Process terminated. Infinite recursion during resource lookup within System.Private.CoreLib.  This may be a bug in System.Private.CoreLib, or potentially in certain extensibility points such as assembly resolve events or CultureInfo names.  Resource name: Argument_InvalidResourceCultureName
   at System.Environment.FailFast(System.String)
   at System.SR.InternalGetResourceString(System.String)
   at System.SR.GetResourceString(System.String, System.String)
   at System.Globalization.CultureInfo.VerifyCultureName(System.String, Boolean)
   at System.Resources.ResourceManager.GetResourceFileName(System.Globalization.CultureInfo)
   at System.Resources.ManifestBasedResourceGroveler.GrovelForResourceSet(System.Globalization.CultureInfo, System.Collections.Generic.Dictionary`2<System.String,System.Resources.ResourceSet>, Boolean, Boolean)
   at System.Resources.ResourceManager.InternalGetResourceSet(System.Globalization.CultureInfo, Boolean, Boolean)
   at System.Resources.ResourceManager.GetString(System.String, System.Globalization.CultureInfo)
   at System.SR.InternalGetResourceString(System.String)
   at System.SR.GetResourceString(System.String, System.String)
   at System.Globalization.CultureInfo.VerifyCultureName(System.String, Boolean)
   at System.Resources.ResourceManager.GetResourceFileName(System.Globalization.CultureInfo)
   at System.Resources.ManifestBasedResourceGroveler.GrovelForResourceSet(System.Globalization.CultureInfo, System.Collections.Generic.Dictionary`2<System.String,System.Resources.ResourceSet>, Boolean, Boolean)
   at System.Resources.ResourceManager.InternalGetResourceSet(System.Globalization.CultureInfo, Boolean, Boolean)
   at System.Resources.ResourceManager.GetString(System.String, System.Globalization.CultureInfo)
   at Microsoft.DotNet.Configurer.DotnetFirstTimeUseConfigurer.PrintFirstTimeMessageWelcome()
   at Microsoft.DotNet.Configurer.DotnetFirstTimeUseConfigurer.Configure()
   at Microsoft.DotNet.Cli.Program.ConfigureDotNetForFirstTimeUse(Microsoft.DotNet.Configurer.IFirstTimeUseNoticeSentinel, Microsoft.DotNet.Configurer.IAspNetCertificateSentinel, Microsoft.DotNet.Configurer.IFileSentinel, Boolean, Microsoft.DotNet.Configurer.DotnetFirstRunConfiguration, Microsoft.DotNet.Cli.Utils.IEnvironmentProvider)
   at Microsoft.DotNet.Cli.Program.ProcessArgs(System.String[], Microsoft.DotNet.Cli.Telemetry.ITelemetry)
   at Microsoft.DotNet.Cli.Program.Main(System.String[])

dotnet/runtime#32510

@coveralls
Copy link

coveralls commented Jun 13, 2020

Coverage Status

Coverage remained the same at 96.692% when pulling 208d166 on github_actions into 166a87d on master.

run: |
if [[ "${{ matrix.lang }}" == "API" ]]; then
export TEST="API";
export COVERALLS_REPO_TOKEN="COVERALLS TOKEN FOR M2CGEN";
Copy link
Member

Choose a reason for hiding this comment

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

Shall we use a Coveralls action (https://github.com/marketplace/actions/coveralls-github-action) instead of manually invoking the coveralls binary in the test.sh script?

@StrikerRUS
Copy link
Member Author

Oh, I just noticed that the Action with R language takes more than 2 hours (all other Actions take approximately the same time as they do at Travis)! It looks like either Travis has better CPUs or Rscript conflicts with some installed software in GitHub Actions image.

Copy link
Member

@izeigerman izeigerman left a comment

Choose a reason for hiding this comment

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

Looks great, thank you! What is our plan with regard to Coveralls and moving away from Travis completely?

@izeigerman izeigerman merged commit 19c13b3 into master Jul 20, 2020
@izeigerman izeigerman deleted the github_actions branch July 20, 2020 16:24
@StrikerRUS
Copy link
Member Author

I'm pretty sure we should use Coveralls Action instead of manual invocation in bash script. But unfortunately, I have no experience with it.

Speaking about the Travis, the only reason I see to keep it is even more parallel jobs (possibly later with greater number of supported languages and models). Maybe we can run r_lang job at Travis as it takes much less time there? Or it is overkill?..

@izeigerman
Copy link
Member

Maybe we can run r_lang job at Travis as it takes much less time there? Or it is overkill?.

That's actually a very interesting idea, tough I'd rather prefer to have a consistent and uniform environment for all our tests. We should keep this in mind.

I'm pretty sure we should use Coveralls Action instead of manual invocation in bash script.

I'll take a look into this one

@StrikerRUS
Copy link
Member Author

So, we are removing Travis config for now (after migrating to Coveralls Action), right?

Also, please don't forget to make GitHub Actions checks required: #243 (comment).

One more frustrating thing is that each time config (matrix or jobs name) changes, you need to go to repo settings and update the list of required checks. Refer to microsoft/LightGBM#3188 (comment) (paragraph starting with GitHub Actions uses ...). For example, #262 changes the set of jobs in matrix which means you'll need to update required checks.

@izeigerman
Copy link
Member

So, we are removing Travis config for now (after migrating to Coveralls Action), right?

Yeah, let's do that.

@StrikerRUS
Copy link
Member Author

tough I'd rather prefer to have a consistent and uniform environment for all our tests.

BTW, we can use our Docker file for that purpose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants