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

sage --tox -e coverage.py #36505

Merged
merged 11 commits into from
Dec 6, 2023
Merged

sage --tox -e coverage.py #36505

merged 11 commits into from
Dec 6, 2023

Conversation

mkoeppe
Copy link
Member

@mkoeppe mkoeppe commented Oct 22, 2023

We add the Coverage.py analysis, already part of our Build&Test CI, as a tox environment, and add documentation.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies


*Documentation:* https://coverage.readthedocs.io


Copy link
Collaborator

Choose a reason for hiding this comment

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

As the doctest coverage follows the code coverage right below, you may want to write something below to distinguish one from the other.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, done in 657c624

envdir = {[sage_src]envdir}
commands = {[sage_src]commands}
allowlist_externals = {[sage_src]allowlist_externals}

[testenv:coverage]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I get

$ ./sage -tox -e coverage.py-html
ROOT: HandledError| provided environments not found in configuration file:
coverage.py-html

Copy link
Member Author

@mkoeppe mkoeppe Oct 24, 2023

Choose a reason for hiding this comment

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

What does ./sage -tox --version say on your system?

Edit: Never mind, I can reproduce it here with tox 4.11.3

Copy link
Member Author

@mkoeppe mkoeppe Oct 24, 2023

Choose a reason for hiding this comment

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

tox-dev/tox#3096 may be relevant for this.
I'll work around it.

(Edit: Done in 5887b53)

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 24, 2023

In the report, I see many instances of

Combined data file .coverage/.coverage.Helios.local.70384.568987
Skipping duplicate data .coverage/.coverage.Helios.local.71097.566208
Skipping duplicate data .coverage/.coverage.Helios.local.71074.300096
Combined data file .coverage/.coverage.Helios.local.38842.870101
/Users/kwankyu/GitHub/sage-dev/local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/coverage/data.py:171: CoverageWarning: Data file '/Users/kwankyu/GitHub/sage-dev/src/.coverage/.coverage.Helios.local.39475.460681' doesn't seem to be a coverage data file: 
  data._warn(str(exc))
Couldn't combine data file .coverage/.coverage.Helios.local.39475.460681: Data file '/Users/kwankyu/GitHub/sage-dev/src/.coverage/.coverage.Helios.local.39475.460681' doesn't seem to be a coverage data file: 
Combined data file .coverage/.coverage.Helios.local.47250.819342
Combined data file .coverage/.coverage.Helios.local.58035.978753
Skipping duplicate data .coverage/.coverage.Helios.local.39256.038416

and at the end, I get

sage/topology/simplicial_set.py                                                        671     29    96%
sage/topology/simplicial_set_catalog.py                                                  2      0   100%
sage/topology/simplicial_set_constructions.py                                          750     80    89%
sage/topology/simplicial_set_examples.py                                               264     13    95%
sage/topology/simplicial_set_morphism.py                                               335     28    92%
sage/typeset/all.py                                                                      3      0   100%
sage/typeset/ascii_art.py                                                               21      1    95%
sage/typeset/character_art.py                                                          165     23    86%
sage/typeset/character_art_factory.py                                                  143      6    96%
sage/typeset/symbols.py                                                                 54      1    98%
sage/typeset/unicode_art.py                                                             27      1    96%
sage/typeset/unicode_characters.py                                                       9      0   100%
sage/version.py                                                                          3      0   100%
--------------------------------------------------------------------------------------------------------
TOTAL                                                                               405317  45798    89%
  coverage.py: FAIL code 5 (2373.06=setup[0.71]+cmd[3.44,2273.05,62.51,33.35] seconds)
  evaluation failed :( (2373.16 seconds)

@mkoeppe
Copy link
Member Author

mkoeppe commented Oct 24, 2023

In the report, I see many instances of

Combined data file .coverage/.coverage.Helios.local.70384.568987
Skipping duplicate data .coverage/.coverage.Helios.local.71097.566208
Skipping duplicate data .coverage/.coverage.Helios.local.71074.300096

We also see these messages in large quantities the Build & Test CI runs, in the section "Prepare coverage results". For example https://github.com/sagemath/sage/actions/runs/6623852735/job/17991733088#step:19:20

/Users/kwankyu/GitHub/sage-dev/local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/coverage/data.py:171: CoverageWarning: Data file '/Users/kwankyu/GitHub/sage-dev/src/.coverage/.coverage.Helios.local.39475.460681' doesn't seem to be a coverage data file: 
  data._warn(str(exc))
Couldn't combine data file .coverage/.coverage.Helios.local.39475.460681: Data file '/Users/kwankyu/GitHub/sage-dev/src/.coverage/.coverage.Helios.local.39475.460681' doesn't seem to be a coverage data file: 

This one also shows up in the CI, for example https://github.com/sagemath/sage/actions/runs/6606391152/job/17942563622#step:19:1162, but with much less frequency.

This looks to me like a concurrency bug. It may, in fact, help explain the nondeterministic coverage results that we observed even after fixing the random seed. (Edit: See #35522)

@tobiasdiez any insights on this?

@tobiasdiez
Copy link
Contributor

This looks to me like a concurrency bug. It may, in fact, help explain the nondeterministic coverage results that we observed even after fixing the random seed. (Edit: See #35522)

@tobiasdiez any insights on this?

Not really. I don't remember seeing it when I introduced the codecov feature, but it might have been there. At least, the oldest available github log also shows it: https://github.com/sagemath/sage/actions/runs/5706566264/job/15462390831
(This run also shows that the issue is unrelated to the problem that the upload is taking hours now, that's an issue introduced probably by your recent changes.)

But you can have a look at the reported coverage file and see how/where it duplicates with others? In general, coverage works way better with pytest.

@mkoeppe
Copy link
Member Author

mkoeppe commented Oct 25, 2023

I've opened #36539 for this.

@mkoeppe
Copy link
Member Author

mkoeppe commented Oct 25, 2023

Let's get this in please.

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 27, 2023

I get

coverage.py: commands[0] /Users/kwankyu/GitHub/sage-dev/src> ../sage --python -m coverage run /Users/kwankyu/GitHub/sage-dev/src/../venv/bin/sage-runtests -p 0 --all
/Users/kwankyu/GitHub/sage-dev/venv/bin/sage-runtests:4: DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html
  __import__('pkg_resources').require('sagemath-standard==10.2b8')

@mkoeppe
Copy link
Member Author

mkoeppe commented Oct 27, 2023

This comes from our old-fashioned installation of the Sage library using "setup.py develop" for the editable mode. The helper script installed as venv/bin/sage-runtests contains this use of pkg_resources. This will go away with #34209

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 27, 2023

From ./sage -tox -e coverage.py-html, there are new doctest failures like

sage -t --warn-long 43.0 --random-seed=23857557043276574322430199400396158591 sage/repl/inputhook.py
**********************************************************************
File "sage/repl/inputhook.py", line 57, in sage.repl.inputhook.install
Failed example:
    get_test_shell()
Expected:
    <sage.repl.interpreter.SageTestShell object at ...>
Got:
    doctest:warning
      File "<frozen runpy>", line 198, in _run_module_as_main
      File "<frozen runpy>", line 88, in _run_code
      ....
      File "/Users/kwankyu/GitHub/sage-dev/local/var/lib/sage/venv-python3.11/lib/python3.11/site-packages/traitlets/config/configurable.py", line 551, in instance
        inst = cls(*args, **kwargs)
      File "/Users/kwankyu/GitHub/sage-dev/local/var/lib/sage/venv-python3.11/lib/python3.11/site-packages/IPython/terminal/interactiveshell.py", line 651, in __init__
        super(TerminalInteractiveShell, self).__init__(*args, **kwargs)
      File "/Users/kwankyu/GitHub/sage-dev/local/var/lib/sage/venv-python3.11/lib/python3.11/site-packages/IPython/core/interactiveshell.py", line 570, in __init__
        self.init_virtualenv()
      File "/Users/kwankyu/GitHub/sage-dev/local/var/lib/sage/venv-python3.11/lib/python3.11/site-packages/IPython/core/interactiveshell.py", line 882, in init_virtualenv
        warn(
      File "/usr/local/Cellar/python@3.11/3.11.5/Frameworks/Python.framework/Versions/3.11/lib/python3.11/warnings.py", line 109, in _showwarnmsg
        sw(msg.message, msg.category, msg.filename, msg.lineno,
    :
    UserWarning: Attempting to work in a virtualenv. If you encounter problems, please install IPython inside the virtualenv.
    <sage.repl.interpreter.SageTestShell object at 0x1741fd390>
**********************************************************************
1 item had failures:
   1 of   6 in sage.repl.inputhook.install
    [7 tests, 1 failure, 0.12 s]

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 27, 2023

TOTAL                                                                               405317  45609    89%
  coverage.py-html: FAIL code 5 (4289.23=setup[0.11]+cmd[4.16,4156.63,85.21,43.12] seconds)
  evaluation failed :( (4289.33 seconds)

Is this expected or normal?

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 27, 2023

This ./sage -tox -e coverage.py-html should generate an html report? Where is the generated report? There's no message indicating where that can be found...

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 14, 2023

This ./sage -tox -e coverage.py-html should generate an html report? Where is the generated report? There's no message indicating where that can be found...

Fixed it now. It prints this at the end:

sage/typeset/unicode_art.py                                                        27     14    48%
sage/typeset/unicode_characters.py                                                  9      0   100%
sage/version.py                                                                     3      0   100%
---------------------------------------------------------------------------------------------------
TOTAL                                                                          196885 156219    21%
coverage.py-html: commands_post[2] /Users/mkoeppe/s/sage/sage-rebasing/worktree-clean/src> ../sage --python -m coverage html -d /Users/mkoeppe/s/sage/sage-rebasing/worktree-clean/src/.tox/sagedirect
Wrote HTML report to /Users/mkoeppe/s/sage/sage-rebasing/worktree-clean/src/.tox/sagedirect/index.html
  coverage.py-html: OK (103.46=setup[0.09]+cmd[2.87,10.30,0.86,17.70,71.64] seconds)
  congratulations :) (103.57 seconds)

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 14, 2023

I see

        coverage.py            -- run the Sage doctester with Coverage.py 
                                  (use "sage --tox -e coverage.py-html" to generate HTML report)
        coverage.py-html       -- run the Sage doctester with Coverage.py 
                                  (use "sage --tox -e coverage.py-html" to generate HTML report)

from the help.

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 14, 2023

Thanks, fixed in aa53aad

Copy link

Documentation preview for this PR (built with commit aa53aad; changes) is ready! 🎉

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 15, 2023

I still get

...
sage/typeset/unicode_art.py                                                             27      1    96%
sage/typeset/unicode_characters.py                                                       9      0   100%
sage/version.py                                                                          3      0   100%
--------------------------------------------------------------------------------------------------------
TOTAL                                                                               405606  45755    89%
coverage.py-html: commands_post[2] /Users/kwankyu/GitHub/sage-dev/src> ../sage --python -m coverage html -d /Users/kwankyu/GitHub/sage-dev/src/.tox/sagedirect
Wrote HTML report to /Users/kwankyu/GitHub/sage-dev/src/.tox/sagedirect/index.html
  coverage.py-html: FAIL code 5 (4454.56=setup[0.82]+cmd[5.92,4135.39,88.54,42.95,180.94] seconds)
  evaluation failed :( (4454.77 seconds)

Perhaps because of many

/Users/kwankyu/GitHub/sage-dev/local/var/lib/sage/venv-python3.11/lib/python3.11/site-packages/coverage/data.py:171: CoverageWarning: Data file '/Users/kwankyu/GitHub/sage-dev/src/.coverage/.coverage.Hera.local.96788.746800' doesn't seem to be a coverage data file: 
  data._warn(str(exc))
Couldn't combine data file .coverage/.coverage.Hera.local.96788.746800: Data file '/Users/kwankyu/GitHub/sage-dev/src/.coverage/.coverage.Hera.local.96788.746800' doesn't seem to be a coverage data file: 
Skipping duplicate data .coverage/.coverage.Hera.local.29006.031604

Since I get the coverage report (may not be accurate), I may simply ignore this, if this is unfixable now...

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 15, 2023

It's not the same error messages as in #36539, but I'd think it's the same reason of failure

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 15, 2023

In this old thread nedbat/coveragepy#408, people experience a similar error and blame some version conflict.

Copy link
Collaborator

@kwankyu kwankyu left a comment

Choose a reason for hiding this comment

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

OK. Let's get this in now.

The new feature does not always work. But the cause seems to be on Coverage.py side.

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 15, 2023

Thank you!

vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 4, 2023
    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
We add the Coverage.py analysis, already part of our Build&Test CI, as a
tox environment, and add documentation.

<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36505
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
@vbraun vbraun merged commit d67dba4 into sagemath:develop Dec 6, 2023
16 of 20 checks passed
@mkoeppe mkoeppe added this to the sage-10.3 milestone Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants