Skip to content

Renaming CI legs#3553

Merged
Anipik merged 4 commits intodotnet:masterfrom
Anipik:ciLegsRename
Apr 26, 2019
Merged

Renaming CI legs#3553
Anipik merged 4 commits intodotnet:masterfrom
Anipik:ciLegsRename

Conversation

@Anipik
Copy link
Contributor

@Anipik Anipik commented Apr 23, 2019

The previous pr was alwayus using the mac machines with missing dependencies so just creating a new pr to avoid those failures.

@eerhardt is it fine to run centos just with netcoreapp3.0 and ubuntu with netcoreapp2.1 ?

@Anipik
Copy link
Contributor Author

Anipik commented Apr 23, 2019

@artidoro the mac ci legs are still failing

@codecov
Copy link

codecov bot commented Apr 23, 2019

Codecov Report

Merging #3553 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3553      +/-   ##
==========================================
- Coverage   72.77%   72.76%   -0.01%     
==========================================
  Files         808      808              
  Lines      145452   145452              
  Branches    16244    16244              
==========================================
- Hits       105846   105839       -7     
- Misses      35185    35192       +7     
  Partials     4421     4421
Flag Coverage Δ
#Debug 72.76% <ø> (-0.01%) ⬇️
#production 68.27% <ø> (-0.01%) ⬇️
#test 89.04% <ø> (-0.01%) ⬇️
Impacted Files Coverage Δ
src/Microsoft.ML.Transforms/Text/LdaTransform.cs 89.26% <0%> (-0.63%) ⬇️
...soft.ML.TestFramework/DataPipe/TestDataPipeBase.cs 73.73% <0%> (-0.33%) ⬇️
...soft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs 84.7% <0%> (-0.21%) ⬇️
src/Microsoft.ML.TimeSeries/IidSpikeDetector.cs 72.54% <0%> (ø) ⬆️
src/Microsoft.ML.Transforms/CategoricalCatalog.cs 68.42% <0%> (ø) ⬆️
src/Microsoft.ML.Data/Transforms/Hashing.cs 92.32% <0%> (ø) ⬆️
...dardTrainers/Standard/Online/AveragedPerceptron.cs 89.7% <0%> (ø) ⬆️
...rosoft.ML.Transforms/FourierDistributionSampler.cs 84.16% <0%> (ø) ⬆️
src/Microsoft.ML.PCA/PcaTrainer.cs 79.94% <0%> (ø) ⬆️
...LogisticRegression/MulticlassLogisticRegression.cs 67.61% <0%> (ø) ⬆️
... and 51 more

@Anipik Anipik requested a review from danmoseley April 24, 2019 00:16
@artidoro
Copy link
Contributor

Looks a lot better already!

I have the following comments:

  • Could you Pascal case the names? Netcoreapp -> NetCoreApp
  • Could you remove the app from the name? NetCoreApp -> NetCore
  • Could you do the same for netfx? netfx -> NetFx
  • Could you specify which version of NetFx we are using? (if you think it's useful)

@artidoro
Copy link
Contributor

On a separate note, is it possible to add a period in the name?
For example NetCore_3.0 or NetCore3.0 instead of NetCore30.

@Anipik
Copy link
Contributor Author

Anipik commented Apr 24, 2019

Could you remove the app from the name? NetCoreApp -> NetCore

Actually eric told me to keep that as we dont use netcore anywhere

Could you do the same for netfx? netfx -> NetFx

Its actually azure thats changing it automatiically from uppercase to lowercase

Could you specify which version of NetFx we are using? (if you think it's useful)

Its not that useful but I can add that, as it may fix our lowercase problem

@artidoro artidoro closed this Apr 24, 2019
@artidoro artidoro reopened this Apr 24, 2019
@artidoro
Copy link
Contributor

artidoro commented Apr 24, 2019

It is strange that it is not keeping the Pascal case on NetCoreApp!
Do you have any ideas on how to fix that?

@Anipik
Copy link
Contributor Author

Anipik commented Apr 24, 2019

It is strange that it is not keeping the Pascal case on NetCoreApp!

It does weird things :P

parameters:
name: Windows_NetFx
name: Windows_x64_NetFx461
buildScript: build.cmd
Copy link
Member

Choose a reason for hiding this comment

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

explicit architecture: x64 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

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

Seems OK to me. Next time if you can avoid reordering, the diff is easier to read.

@Anipik
Copy link
Contributor Author

Anipik commented Apr 26, 2019

Next time if you can avoid reordering, the diff is easier to read.

Actually ML team wanted these reorderings. but yeah i will split the reordering into a separate commit from next time

@eerhardt
Copy link
Member

Why do we need “Build” in all the names?

@Anipik
Copy link
Contributor Author

Anipik commented Apr 26, 2019

Why do we need “Build” in all the names?

chocsta mentioned in the previous PR and naming the job matrices as debug and release can cause some problems with azure dev ops #3314 (comment)

@eerhardt
Copy link
Member

@eerhardt is it fine to run centos just with netcoreapp3.0 and ubuntu with netcoreapp2.1 ?

It was nice getting a mix of the two that we were doing previously, but I think we should be OK.

@Anipik Anipik merged commit 802edc7 into dotnet:master Apr 26, 2019
@Anipik Anipik deleted the ciLegsRename branch April 26, 2019 18:21
@shauheen
Copy link
Contributor

@Anipik this seems to have broken the build badges on the main readme.

@Anipik
Copy link
Contributor Author

Anipik commented Apr 28, 2019

@shauheen We need to update the job names there as well. i Will put up the PR for it.

@Anipik
Copy link
Contributor Author

Anipik commented Apr 28, 2019

We need to update the links

@Anipik
Copy link
Contributor Author

Anipik commented Apr 28, 2019

i found the new links to apis

@ghost ghost locked as resolved and limited conversation to collaborators Sep 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants