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

Repair sage -t --valgrind #37569

Merged
merged 16 commits into from
Sep 15, 2024
Merged

Repair sage -t --valgrind #37569

merged 16 commits into from
Sep 15, 2024

Conversation

mkoeppe
Copy link
Member

@mkoeppe mkoeppe commented Mar 8, 2024

As noted in #36046 (comment), sage -t --valgrind does not work because it instruments the wrong process.

As part of the fix, we move the contents of src/bin/sage-runtests to src/bin/sage/doctest/__main__.py so that the doctester can be invoked as sage -python -m sage.doctest.

We also arrange for sage -t --valgrind to use the suppressions file added in #36046.

📝 Checklist

  • The title is concise and informative.
  • 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

@antonio-rojas
Copy link
Contributor

Works fine with my current testing workflow (no --valgrind). However, using --valgrind doesn't work:

> sage -t --installed --valgrind
too many failed tests, not using stored timings
exec valgrind --tool=memcheck --leak-resolution=high --leak-check=full --num-callers=25 --suppressions=/usr/lib/python3.11/site-packages/sage/ext_data/valgrind/pyalloc.supp --suppressions=/usr/lib/python3.11/site-packages/sage/ext_data/valgrind/sage.supp --suppressions=/usr/lib/python3.11/site-packages/sage/ext_data/valgrind/sage-additional.supp --suppressions=/usr/lib/python3.11/site-packages/sage/ext_data/valgrind/valgrind-python.supp  --log-file=/home/antonio/.sage/valgrind/sage-memcheck.%p /usr/bin/python -m sage.doctest --serial --timeout=172800 --stats_path=/home/antonio/.sage/timings2.json --optional=pip,sage 
either use --new, --all, --installed, or some filenames

(which is nonsense, since I did pass --installed)

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 11, 2024

Thanks for testing! I'll investigate.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 11, 2024

This quick fix should take care of --installed, but there is a whole list of other options that would also need to be handled.

@antonio-rojas
Copy link
Contributor

This quick fix should take care of --installed, but there is a whole list of other options that would also need to be handled.

Since --all is disallowed, shouldn't --installed be disallowed too? Since usually one does have the entire library installed, not just one file.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 15, 2024

Since --all is disallowed, shouldn't --installed be disallowed too?

I've removed this check now. I think it was poorly motivated. Note that one could already pass multiple filenames on the command line.

@mkoeppe
Copy link
Member Author

mkoeppe commented Mar 15, 2024

there is a whole list of other options that would also need to be handled.

I've added them all now.

@mkoeppe mkoeppe removed this from the sage-10.3 milestone Mar 20, 2024
@mkoeppe mkoeppe force-pushed the doctest_valgrind branch 2 times, most recently from e966332 to 479ad45 Compare April 1, 2024 00:51
Copy link

github-actions bot commented Apr 1, 2024

Documentation preview for this PR (built with commit 01ce6d7; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 13, 2024

I should learn to set a "needs work" PR back to "draft" before rebasing it

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 13, 2024

Ready? As far as I can see, it is working well.

@mkoeppe mkoeppe marked this pull request as ready for review August 13, 2024 06:11
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, though I have no skill in interpreting the logs generated by the test run of valgrind.

@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 13, 2024

Thanks!

vbraun pushed a commit to vbraun/sage that referenced this pull request Aug 27, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

As noted in
sagemath#36046 (comment),
`sage -t --valgrind` does not work because it instruments the wrong
process.

As part of the fix, we move the contents of `src/bin/sage-runtests` to
`src/bin/sage/doctest/__main__.py` so that the doctester can be invoked
as `sage -python -m sage.doctest`.

We also arrange for `sage -t --valgrind` to use the suppressions file
added in sagemath#36046.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] 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. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37569
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
@vbraun
Copy link
Member

vbraun commented Aug 27, 2024

I'm getting

**********************************************************************
File "src/sage/doctest/__main__.py", line 38, in sage.doctest.__main__._get_optional_defaults
Failed example:
    DA = DocTestDefaults(runtest_default=True, **D); DA
Expected:
    DocTestDefaults(abspath=False, file_iterations=0, global_iterations=0,
                    optional='sage,optional', random_seed=None,
                    stats_path='.../timings2.json')
Got:
    DocTestDefaults(abspath=False, file_iterations=0, global_iterations=0, optional='sage,optional', stats_path='/var/lib/buildbot/worker/sage_git/dot_sage/timings2.json')
**********************************************************************

@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 27, 2024

Do you set doctester-related env variables or pass options to sage -t on buildbot?

@vbraun
Copy link
Member

vbraun commented Aug 27, 2024

Buildbot sets SAGE_DOCTEST_RANDOM_SEED=0

@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 27, 2024

Thanks, I'll investigate

vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 5, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

As noted in
sagemath#36046 (comment),
`sage -t --valgrind` does not work because it instruments the wrong
process.

As part of the fix, we move the contents of `src/bin/sage-runtests` to
`src/bin/sage/doctest/__main__.py` so that the doctester can be invoked
as `sage -python -m sage.doctest`.

We also arrange for `sage -t --valgrind` to use the suppressions file
added in sagemath#36046.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] 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. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37569
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 6, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

As noted in
sagemath#36046 (comment),
`sage -t --valgrind` does not work because it instruments the wrong
process.

As part of the fix, we move the contents of `src/bin/sage-runtests` to
`src/bin/sage/doctest/__main__.py` so that the doctester can be invoked
as `sage -python -m sage.doctest`.

We also arrange for `sage -t --valgrind` to use the suppressions file
added in sagemath#36046.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] 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. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37569
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 8, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

As noted in
sagemath#36046 (comment),
`sage -t --valgrind` does not work because it instruments the wrong
process.

As part of the fix, we move the contents of `src/bin/sage-runtests` to
`src/bin/sage/doctest/__main__.py` so that the doctester can be invoked
as `sage -python -m sage.doctest`.

We also arrange for `sage -t --valgrind` to use the suppressions file
added in sagemath#36046.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] 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. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37569
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 8, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

As noted in
sagemath#36046 (comment),
`sage -t --valgrind` does not work because it instruments the wrong
process.

As part of the fix, we move the contents of `src/bin/sage-runtests` to
`src/bin/sage/doctest/__main__.py` so that the doctester can be invoked
as `sage -python -m sage.doctest`.

We also arrange for `sage -t --valgrind` to use the suppressions file
added in sagemath#36046.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] 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. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37569
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 10, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

As noted in
sagemath#36046 (comment),
`sage -t --valgrind` does not work because it instruments the wrong
process.

As part of the fix, we move the contents of `src/bin/sage-runtests` to
`src/bin/sage/doctest/__main__.py` so that the doctester can be invoked
as `sage -python -m sage.doctest`.

We also arrange for `sage -t --valgrind` to use the suppressions file
added in sagemath#36046.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] 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. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37569
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 12, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

As noted in
sagemath#36046 (comment),
`sage -t --valgrind` does not work because it instruments the wrong
process.

As part of the fix, we move the contents of `src/bin/sage-runtests` to
`src/bin/sage/doctest/__main__.py` so that the doctester can be invoked
as `sage -python -m sage.doctest`.

We also arrange for `sage -t --valgrind` to use the suppressions file
added in sagemath#36046.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] 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. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37569
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
@vbraun vbraun merged commit d1fe412 into sagemath:develop Sep 15, 2024
17 of 19 checks passed
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.

4 participants