feat: new COVERAGEPY_IGNORE_ERRORS env var

[BurnzZ]

Background

This is a proposal to have a workaround for #2575 where an inherent issue from coverage.py when creating the lcov output file.

Proposed Changes

An env var flag that a user can turn on via --test_env=COVERAGEPY_IGNORE_ERRORS=true when running bazel coverage.

Reproducible steps

We can see how this proposal addresses the issue described from https://github.com/BurnzZ/rules_python.

1. Clone the repo: https://github.com/BurnzZ/rules_python

git clone https://github.com/BurnzZ/rules_python.git rules_python_tmp
cd rules_python_tmp/examples
git checkout feat-new-env-var-COVERAGEPY_IGNORE_ERRORS

2. Clone the reproducible example

git clone https://github.com/BurnzZ/bazel-python-coverage-issue.git
cd bazel-python-coverage-issue
git checkout enable-torchvision

3. Add these lines in line 2 of MODULE.bazel:

local_path_override(
module_name = "rules_python",
path = "../..",
)

4. Generate the coverage report:

bazel coverage --combined_report=lcov :test --nocache_test_results --test_output=all --test_env=COVERAGEPY_IGNORE_ERRORS=true
lcov --list "$(bazel info output_path)/_coverage/_coverage_report.dat"
genhtml --branch-coverage --output genhtml "$(bazel info output_path)/_coverage/_coverage_report.dat"

Issue from #2575 now resolved. 🎉

[groodt]

I think it makes sense to provide a mechanism to let coveragepy ignore errors.

I also wonder if it makes sense to provide a way for all of the coveragepy CLI arguments to be supplied?

I think the torch errors that motivate this PR can probably also be avoided by excluding that source from coverage. Ignoring all errors would avoid the torch error, but possibly also mask others.

To be clear, I still think there are valid scenarios to ignore all, so this PR still LGTM.

WDYT @rickeylev

[groodt]

LGTM

[BurnzZ]

I also wonder if it makes sense to provide a way for all of the coveragepy CLI arguments to be supplied?

This was initially my approach but since there are 2 runs: coverage run and coverage lcov, we need to consider that the args could be exclusive to each. On the other hand, there might be cases that they should be the same to ensure proper processing of the coverage artifacts. I haven't thought deeply about the other use cases for this yet.

I think the torch errors that motivate this PR can probably also be avoided by excluding that source from coverage.

From coveragepy/coveragepy#1921 (comment), I think an alternative would be coverage run --omit="$TMPDIR/*" test_foo.py. Will need to properly test this out.

Ignoring all errors would avoid the torch error, but possibly also mask others.

Yeah, that's indeed another risk.

Although, I think we should be fine since --ignore-errors"tells coverage.py to ignore problems encountered trying to find source files to report on. This can be useful if some files are missing, or if your Python execution is tricky enough that file names are synthesized without real source files" (reference). We still get useful warning messages (unless --disable_warning is added).

[rickeylev]

How about enabling ignore errors by default instead?

[BurnzZ]

That's one option but it could hide other potential issues by default.

I think it'd be best if the user can first see the error, then decide if errors are to be ignored. This opt-in approach is sufficient for the time being.

[timothy-tlj-jones-canva]

I heartily endorse this

[aignas]

Is this needed if we have #2607?

[BurnzZ]

We can close this since #2607 is on the way.

Although there are some benefits in having an escape hatch like --ignore-errors could provide outside of #2575. We can revisit if needed.