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 --fixdoctests --update-known-test-failures; silence modularized distributions in CI #36264

Merged

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Sep 13, 2023

The distributions sagemath-repl and sagemath-categories run parts of the testsuite in a virtual environment created by tox when SAGE_CHECK=yes. This also runs as part of the Build & Test CI.

Here we make several improvements:

  • Support SAGE_CHECK=warn
  • Invoke the doctester with --baseline-stats-path={toxinidir}/known-test-failures.json
  • Make the tests silent unless new test failures, not recorded in the known-test-failures.json, are encountered

To help maintain the known-test-failures.json, we also add new features to the command sage --fixdoctests.

  • sage --fixdoctests --update-known-test-failures reads the stats files generated by the doctester in the virtual environments and writes updated known-test-failures.json files to the source tree
  • doctester stats now also include a field ntests - number of doctests of a module that were run

sage --fixdoctests also receives new switches --distribution all, --fixed-point, --verbose, --no-diff and some other improvements.

📝 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

@mkoeppe mkoeppe force-pushed the ci_build_silence_modularized_distributions branch from d603403 to 834ad0a Compare September 16, 2023 18:31
[testenv:sagepython-sagewheels]
basepython = {env:SAGE_VENV}/bin/python
package_env = .pkg-sagepython

[testenv:sagepython-norequirements]
basepython = {env:SAGE_VENV}/bin/python3
package_env = .pkg-sagepython


[testenv:sagepython-sagewheels-nopypi-norequirements]
Copy link
Collaborator

Choose a reason for hiding this comment

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

single empty line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This break is intended - this sets apart the default environment

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK.

[testenv:sagepython-sagewheels]
basepython = {env:SAGE_VENV}/bin/python
package_env = .pkg-sagepython

[testenv:sagepython-norequirements]
basepython = {env:SAGE_VENV}/bin/python3
package_env = .pkg-sagepython


[testenv:sagepython-sagewheels-nopypi-norequirements]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Two lines, intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this separates the default environment from the others above

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK.

},
"sage.repl.attach": {
"ntests": 128
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

No "failed: false"?

Or "success: true"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the timings2.json files created by the doctester don't have these.

# Build dependencies according to requirements.txt (all versions fixed).
# Use ONLY the wheels built and stored by the Sage distribution (no PyPI):
#
# ./sage -sh -c '(cd pkgs/sagemath-standard && tox -v -v -v -e sagepython-sagewheels-nopypi)'
Copy link
Collaborator

Choose a reason for hiding this comment

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

No sagepython-sagewheels-nopypi here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the envlist only lists the default environments, i.e., those that will be tested if one uses tox without the -e switch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK. Thanks.

# ./sage -sh -c '(cd pkgs/sagemath-standard && tox -v -v -v -e sagepython-sagewheels-nopypi)'
#
sagepython-sagewheels-nopypi,
#
Copy link
Collaborator

Choose a reason for hiding this comment

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

    #
    # SUPPORTED ENVIRONMENTS:
    #

No?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed the format from "SUPPORTED" / "OTHER SUPPORTED" to (unmarked) default / "OTHER SUPPORTED"

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 20, 2023

I got

$ sage --fixdoctests
sage-fixdoctests: At least one filename is required when --update-known-test-failures is not used
Running "sage -t -p "

Is it really running "sage -t -p "?

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 20, 2023

 $ sage --fixdoctests --help
...
  --probe FEATURES      check whether '# optional/needs' tags are still
                        needed, remove these
...

remove those not needed?

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 20, 2023

From

$ sage --fixdoctests --update-known-test-failures --verbose
Running "sage -t -p "

I don't understand the output. It seems not running anything.

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 20, 2023

"--fixed-point" sounds good by itself. But since it follows "--fixdoctests", it makes me stop and think. An alternative is "--stabilize". Just a suggestion.

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 20, 2023

$ sage --fixdoctests --distribution sagemath-objects --verbose src/sage/rings/function_field
Running "pkgs/sagemath-objects/.tox/sagepython-sagewheels-nopypi-norequirements/bin/sage -t --environment sage.all__sagemath_objects ./src/sage/version.py"
sh: pkgs/sagemath-objects/.tox/sagepython-sagewheels-nopypi-norequirements/bin/sage: No such file or directory
sage-fixdoctests: Doctester exited with error status 127

Is this normal? I ran this after make wheels.

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 20, 2023

$ sage --fixdoctests --distribution sagemath-categories --verbose src/sage/rings/function_field --fixed-point
##############################################################################
sage-fixdoctests: Fixing with --distribution=sagemath-categories
Running "pkgs/sagemath-categories/.tox/sagepython-sagewheels-nopypi-norequirements/bin/sage -t --environment sage.all__sagemath_categories ./src/sage/version.py"
Running "pkgs/sagemath-categories/.tox/sagepython-sagewheels-nopypi-norequirements/bin/sage -t -p --environment sage.all__sagemath_categories --if-installed src/sage/rings/function_field/all.py src/sage/rings/function_field/constructor.py src/sage/rings/function_field/derivations.py src/sage/rings/function_field/derivations_polymod.py 
...
src/sage/rings/function_field/valuation_ring.py"
usage: sage -t [options] filenames
sage-runtests: error: unrecognized arguments: --if-installed
sage-fixdoctests: Processing unprocessed files
sage-fixdoctests: No fixes made in 'src/sage/rings/function_field/place.py'
...
sage-fixdoctests: Fixed point reached

There is an error sage-runtests: error: unrecognized arguments: --if-installed above.

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 20, 2023

From

$ sage --fixdoctests --distribution sagemath-categories --verbose src/sage/rings/function_field/function_field.py --fixed-point
##############################################################################
sage-fixdoctests: Fixing with --distribution=sagemath-categories
Running "pkgs/sagemath-categories/.tox/sagepython-sagewheels-nopypi-norequirements/bin/sage -t --environment sage.all__sagemath_categories ./src/sage/version.py"
Running "pkgs/sagemath-categories/.tox/sagepython-sagewheels-nopypi-norequirements/bin/sage -t -p --environment sage.all__sagemath_categories --if-installed src/sage/rings/function_field/function_field.py"
usage: sage -t [options] filenames
sage-runtests: error: unrecognized arguments: --if-installed
sage-fixdoctests: Processing unprocessed files
sage-fixdoctests: No fixes made in 'src/sage/rings/function_field/function_field.py'
sage-fixdoctests: Fixed point reached

why does this run?

Running "pkgs/sagemath-categories/.tox/sagepython-sagewheels-nopypi-norequirements/bin/sage -t 
  --environment sage.all__sagemath_categories ./src/sage/version.py"

It seems that this runs if --distribution is given.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 20, 2023

$ sage --fixdoctests
sage-fixdoctests: At least one filename is required when --update-known-test-failures is not used
Running "sage -t -p "

Is it really running "sage -t -p "?

Thanks, fixed in 93f6a05

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 20, 2023

$ sage --fixdoctests --help
...
  --probe FEATURES      check whether '# optional/needs' tags are still
                        needed, remove these
...

remove those not needed?

Thanks, done in e380f9a

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 20, 2023

$ sage --fixdoctests --update-known-test-failures --verbose
Running "sage -t -p "

I don't understand the output. It seems not running anything.

It gives an error now after a61cdbf

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 20, 2023

$ sage --fixdoctests --distribution sagemath-objects --verbose src/sage/rings/function_field
Running "pkgs/sagemath-objects/.tox/sagepython-sagewheels-nopypi-norequirements/bin/sage -t --environment sage.all__sagemath_objects ./src/sage/version.py"
sh: pkgs/sagemath-objects/.tox/sagepython-sagewheels-nopypi-norequirements/bin/sage: No such file or directory
sage-fixdoctests: Doctester exited with error status 127

Is this normal? I ran this after make wheels.

The .tox environments are only created if you run that with SAGE_CHECK=yes (or SAGE_CHECK=warn)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 20, 2023

There is an error sage-runtests: error: unrecognized arguments: --if-installed above.

This must have been an old .tox environment

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 20, 2023

why does this run?

Running "pkgs/sagemath-categories/.tox/sagepython-sagewheels-nopypi-norequirements/bin/sage -t 
  --environment sage.all__sagemath_categories ./src/sage/version.py"

It seems that this runs if --distribution is given.

Before it tests the given files, it uses this command to see if the .tox environment is working. It skips those that do not seem to be working

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 20, 2023

"--fixed-point" sounds good by itself. But since it follows "--fixdoctests", it makes me stop and think. An alternative is "--stabilize". Just a suggestion.

I also considered --until-stable but ultimately I thought that "stable" might be taken as promising something good, whereas "fixed point" expresses that it has converged to "something", which is closer to the truth

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 21, 2023

why does this run?

Running "pkgs/sagemath-categories/.tox/sagepython-sagewheels-nopypi-norequirements/bin/sage -t 
  --environment sage.all__sagemath_categories ./src/sage/version.py"

It seems that this runs if --distribution is given.

Before it tests the given files, it uses this command to see if the .tox environment is working. It skips those that do not seem to be working

Then how about not logging the line "Running ... ./src/sage/version.py", but instead logging the result of the check? That log looks internal.

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 21, 2023

$ sage --fixdoctests --distribution sagemath-objects --verbose src/sage/rings/function_field
Running "pkgs/sagemath-objects/.tox/sagepython-sagewheels-nopypi-norequirements/bin/sage -t --environment 
Is this normal? I ran this after `make wheels`.

The .tox environments are only created if you run that with SAGE_CHECK=yes (or SAGE_CHECK=warn)

The .tox environment is there, but sage is not in bin.

I tested again after SAGE_CHECK=yes make sagemath_objects. I get the same.

This

$sage --fixdoctests --distribution sagemath_categories --verbose src/sage/rings/function_field --fixed-point

works though. So the problem is with sagemath_objects.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 21, 2023

So the problem is with sagemath_objects

Ah, yes. The doctester is provided by sagemath-repl, which depends on sagemath-objects. Because of this, sagemath-objects is not tested using the doctester. (Also, the sage script is provided by sagemath-environment, which is also not installed in the tox environment of sagemath-objects.)

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 21, 2023

I suspected that sort of things. Then how about detecting such a situation, and warn the user of --fixdoctests. If --distribution all is used, then sagemath-objects should be skipped anyway instead of erroring out. Right?

Matthias Koeppe added 21 commits September 21, 2023 15:58
@mkoeppe mkoeppe force-pushed the ci_build_silence_modularized_distributions branch from 7e85c27 to 23e9923 Compare September 21, 2023 22:59
@github-actions
Copy link

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

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.

LGTM.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 22, 2023

Thank you!

@vbraun vbraun merged commit 342e32b into sagemath:develop Sep 24, 2023
@mkoeppe mkoeppe added this to the sage-10.2 milestone Sep 24, 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.

3 participants