You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mxnet.apache.org by GitBox <gi...@apache.org> on 2020/03/23 04:36:17 UTC
[GitHub] [incubator-mxnet] leezu opened a new pull request #17889: Re-enable
coverage on CI for GCC CPU builds
leezu opened a new pull request #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889
As title. Previously https://github.com/apache/incubator-mxnet/pull/15981/ disabled all code coverage builds due to library size limitations on some builds. Let's try to enable code coverage again on the builds not limited by size.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu edited a comment on issue #17889:
Re-enable coverage on CI for GCC CPU builds
Posted by GitBox <gi...@apache.org>.
marcoabreu edited a comment on issue #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889#issuecomment-615850612
Actually the coverage is not stable at all. Look at this example report https://codecov.io/gh/apache/incubator-mxnet/pull/17905/changes which is based on #17905. The PR adds one new operator, but 132 files have coverage changes with a reported line changes in a four digit space.
That example is not ideal (although the lines changed have a coverage of >90%, so the author did a good job of adding test coverage for their new code), but I couldn't find a recent PR which had a successful CI run and was a NO-OP. Maybe just create a few empty PRs, make the CI runs succeed and then you will have a few clear reports which show you exactly which lines are not stable.
Generally we can only consider the coverage report stable if that page reports 0 changes for runs which do not change anything. Otherwise, these numbers are misleading and will confuse the PR authors if we enable the publishing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on issue #17889: Re-enable
coverage on CI for GCC CPU builds
Posted by GitBox <gi...@apache.org>.
leezu commented on issue #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889#issuecomment-604086310
@mxnet-bot run ci [windows-gpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on issue #17889: Re-enable
coverage on CI for GCC CPU builds
Posted by GitBox <gi...@apache.org>.
leezu commented on issue #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889#issuecomment-603965017
@mxnet-bot run ci [windows-gpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu commented on issue #17889: Re-enable
coverage on CI for GCC CPU builds
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on issue #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889#issuecomment-604129225
We could do that, but imo, something even worse than no data is bad data. People will start discarding the results and it will confuse users.
I'd rather recommend to silently record the results and then take the time to write a script which outputs the lines with inconsistent coverage. Then, create tasks as follow-up. Once coverage is consistent, enable publishing
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] mxnet-bot commented on issue #17889: Re-enable
coverage on CI for GCC CPU builds
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on issue #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889#issuecomment-604086350
Jenkins CI successfully triggered : [windows-gpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu commented on issue #17889: Re-enable
coverage on CI for GCC CPU builds
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on issue #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889#issuecomment-615851885
You have to take into account that additionally to randomness in our tests, a stable coverage report requires that the base commit (in the master branch) actually had a successful run. Otherwise, the report is based on partial data.
So I'd say there are two or three action items:
1. Introduce tests for flaky lines
2. Add a retry mechanism for failed CI runs on the master branch
3. Stabilize CI runs to "never". This goal is reached when the master branch CI run rate is close to 100% successful.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu edited a comment on issue #17889:
Re-enable coverage on CI for GCC CPU builds
Posted by GitBox <gi...@apache.org>.
marcoabreu edited a comment on issue #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889#issuecomment-615851885
You have to take into account that additionally to randomness in our tests, a stable coverage report requires that the base commit (in the master branch) actually had a successful run. Otherwise, the report is based on partial data.
So I'd say there are two or three action items:
1. Introduce tests for flaky lines
2. Add a retry mechanism for failed CI runs on the master branch
3. Stabilize CI runs to "never". This goal is reached when the master branch CI run rate is close to 100% successful.
Wrt to 3), I think we're quite far away unfortunately - so 2) is certainly the first thing that should be done:
I'd recommend to spend time on stabilizing CI as highest priority over all the other optimizations.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] szha commented on issue #17889: Re-enable
coverage on CI for GCC CPU builds
Posted by GitBox <gi...@apache.org>.
szha commented on issue #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889#issuecomment-615398344
@leezu just shared with me the coverage data on codecov: https://codecov.io/gh/apache/incubator-mxnet/branch/master. The coverage looks stable and I think the info should be useful. If we see actual randomness in coverage, then we can start asking people to fix the tests that are causing them. @marcoabreu what do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on issue #17889: Re-enable
coverage on CI for GCC CPU builds
Posted by GitBox <gi...@apache.org>.
leezu commented on issue #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889#issuecomment-604026943
@mxnet-bot run ci [windows-gpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu commented on issue #17889: Re-enable
coverage on CI for GCC CPU builds
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on issue #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889#issuecomment-604127553
Coverage is not stable due to the excessive use of random data. Our tests do not cover the same paths every single time. Unless tests are expanded to have a consistent coverage, we can't enable it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu commented on issue #17889: Re-enable
coverage on CI for GCC CPU builds
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on issue #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889#issuecomment-604192321
Sure, please feel free to make a poc once publishing is enabled again and then we can evaluate if the data is helpful or not. Generally I'm a big fan of test coverage diff
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu edited a comment on issue #17889: Re-enable
coverage on CI for GCC CPU builds
Posted by GitBox <gi...@apache.org>.
leezu edited a comment on issue #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889#issuecomment-604158958
Given that it may take significant effort and no effort was spent yet, I don't think it's realistic to expect existing test cases would be rewritten soon. Thus if we take this as a blocker to exposing test coverage, we may never expose it.
If new lines of code are covered by existing test cases, that's still valuable information. We can thereby avoid introducing any completely untested lines of code into the codebase.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu commented on issue #17889: Re-enable
coverage on CI for GCC CPU builds
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on issue #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889#issuecomment-604149521
No, no activities were done besides activating silent collection.
The problem with newly added lines is that you can not make that distinction in an automatic Fashion. The new code might be hit by existing tests based on random data with flaky paths. Also, the system is not able to distinguish new hits and existing hits due to them constantly switching. If the coverage would be consistent, then we could enable exactly that Delta.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on issue #17889: Re-enable
coverage on CI for GCC CPU builds
Posted by GitBox <gi...@apache.org>.
leezu commented on issue #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889#issuecomment-604148386
Was there any prior effort on reducing the coverage variance during the past years? I think data was collected silently until a few months ago?
We can at least ensure that any newly introduced code has sufficient code coverage and does not suffer from the variances you describe. Providing test coverage data in PRs, separately listing the coverage data of newly added lines, seems still helpful.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu commented on issue #17889: Re-enable
coverage on CI for GCC CPU builds
Posted by GitBox <gi...@apache.org>.
marcoabreu commented on issue #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889#issuecomment-615850612
Actually the coverage is not stable at all. Look at this example report https://codecov.io/gh/apache/incubator-mxnet/pull/17905/changes which is based on #17905. The PR adds one new operator, but 132 files have coverage changes with a reported line changes in a four digit space.
That example is not ideal, but I couldn't find a recent PR which had a successful CI run and was a NO-OP. Maybe just create a few empty PRs, make the CI runs succeed and then you will have a few clear reports which show you exactly which lines are not stable.
Generally we can only consider the coverage report stable if that page reports 0 changes for runs which do not change anything. Otherwise, these numbers are misleading and will confuse the PR authors if we enable the publishing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on issue #17889: Re-enable
coverage on CI for GCC CPU builds
Posted by GitBox <gi...@apache.org>.
leezu commented on issue #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889#issuecomment-604128434
Thanks for explaining the reasoning. We can enable it, without marking it as required.
This will expose the problem and make it easier to fix it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu edited a comment on issue #17889: Re-enable
coverage on CI for GCC CPU builds
Posted by GitBox <gi...@apache.org>.
leezu edited a comment on issue #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889#issuecomment-604128434
Thanks for explaining the reasoning. We can enable it, without marking it as required.
This will expose the problem and make it easier to fix it. What do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu edited a comment on issue #17889: Re-enable
coverage on CI for GCC CPU builds
Posted by GitBox <gi...@apache.org>.
leezu edited a comment on issue #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889#issuecomment-604087122
@marcoabreu would it make sense to add a coverage check to the status checks listed by Github? Currently we only list CI success / failure. Also, is there any reason for not adding it earlier (prior to coverage having been disabled)?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on issue #17889: Re-enable
coverage on CI for GCC CPU builds
Posted by GitBox <gi...@apache.org>.
leezu commented on issue #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889#issuecomment-604158958
Given that's it may take significant effort and no effort was spent yet, I don't think it's realistic to expect existing test cases would be rewritten soon. Thus if we take this as a blocker to exposing test coverage, we may never expose it.
If new lines of code are covered by existing test cases, that's still valuable information. We can thereby avoid introducing any completely untested lines of code into the codebase.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu merged pull request #17889: Re-enable
coverage on CI for GCC CPU builds
Posted by GitBox <gi...@apache.org>.
leezu merged pull request #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] leezu commented on issue #17889: Re-enable
coverage on CI for GCC CPU builds
Posted by GitBox <gi...@apache.org>.
leezu commented on issue #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889#issuecomment-604087122
@marcoabreu would it make sense to add a coverage check to the status checks listed by Github? Currently we only list CI success / failure. Is there any reason for not adding it earlier (prior to coverage having been disabled)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] mxnet-bot commented on issue #17889: Re-enable
coverage on CI for GCC CPU builds
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on issue #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889#issuecomment-603965082
Jenkins CI successfully triggered : [windows-gpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] mxnet-bot commented on issue #17889: Re-enable
coverage on CI for GCC CPU builds
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on issue #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889#issuecomment-604027007
Jenkins CI successfully triggered : [windows-gpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu edited a comment on issue #17889:
Re-enable coverage on CI for GCC CPU builds
Posted by GitBox <gi...@apache.org>.
marcoabreu edited a comment on issue #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889#issuecomment-615851885
You have to take into account that additionally to randomness in our tests, a stable coverage report requires that the base commit (in the master branch) actually had a successful run. Otherwise, the report is based on partial data.
So I'd say there are two or three action items:
1. Introduce tests for flaky lines
2. Add a retry mechanism for failed CI runs on the master branch
3. Stabilize CI runs to "never". This goal is reached when the master branch CI run rate is close to 100% successful.
Wrt to 3), I think we're quite far away unfortunately - so 2) is certainly the first thing that should be done:
I'd recommend to spend time on stabilizing CI as highest priority over all the other cost optimizations.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] marcoabreu edited a comment on issue #17889:
Re-enable coverage on CI for GCC CPU builds
Posted by GitBox <gi...@apache.org>.
marcoabreu edited a comment on issue #17889: Re-enable coverage on CI for GCC CPU builds
URL: https://github.com/apache/incubator-mxnet/pull/17889#issuecomment-604192321
Sure, please feel free to make a poc once publishing is enabled again and then we can evaluate if the data is helpful or not. Generally I'm a big fan of test coverage diff - hence the motivation to introduce the publish in the first place
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services