Skip to content

Use GNU make tokenpool protocol to manage parallelism of doctesting#36640

Merged
vbraun merged 9 commits intosagemath:developfrom
mkoeppe:gnumake_tokenpool
Dec 14, 2023
Merged

Use GNU make tokenpool protocol to manage parallelism of doctesting#36640
vbraun merged 9 commits intosagemath:developfrom
mkoeppe:gnumake_tokenpool

Conversation

@mkoeppe
Copy link
Copy Markdown
Contributor

@mkoeppe mkoeppe commented Nov 3, 2023 •
edited
Loading

This is useful when running our doctester in parallel with other build steps, or several doctesters in parallel, as happens for example in make SAGE_CHECK=yes pypi-wheels, and more of that after #35095.

To test:

MAKE="make -j14" make SAGE_NUM_THREADS=100 DEBUG_JOBCLIENT=1 ptest

This will make the doctester attempt to use 100 workers, but it will only get tokens for 14 workers from make.
DEBUG_JOBCLIENT=1 shows what's happening.

Upstream PR:

📝 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

@orlitzky
Copy link
Copy Markdown
Contributor

orlitzky commented Nov 5, 2023

Do we have to make it standard? Other than that I think all it needs is some doc updates:

  • SAGE_NUM_THREADS in src/doc/en/installation/source.rst
  • -p flag in src/bin/sage-runtests
  • A few places in src/doc/en/developer/doctesting.rst

@mkoeppe
Copy link
Copy Markdown
Contributor Author

mkoeppe commented Nov 6, 2023

Do we have to make it standard?

I think nothing is gained by making it optional. It's a very simple Python script

@mkoeppe
Copy link
Copy Markdown
Contributor Author

mkoeppe commented Nov 6, 2023

I've made the documentation changes in 39a1926

orlitzky
orlitzky approved these changes Nov 7, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@orlitzky orlitzky left a comment

Choose a reason for hiding this comment

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

I'm happy with it now, thanks.

Some day we might be able to replace SAGE_NUM_THREADS in the build entirely, but I'm not going to think about it too hard until ninja supports the job server.

@mkoeppe
Copy link
Copy Markdown
Contributor Author

mkoeppe commented Nov 7, 2023

Thank you!

@vbraun
Copy link
Copy Markdown
Member

vbraun commented Dec 9, 2023

Please don't push broken PRs to me, wtf. You are only allowed to remove the needs work after you actually check that it works.

@mkoeppe
Copy link
Copy Markdown
Contributor Author

mkoeppe commented Dec 9, 2023

Sorry, I'll check what went wrong here.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Dec 9, 2023

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

@mkoeppe
Copy link
Copy Markdown
Contributor Author

mkoeppe commented Dec 9, 2023

All green now.

vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 10, 2023
7e65249 
sagemathgh-36640: Use GNU make tokenpool protocol to manage parallelism of doctesting
    
<!-- ^^^^^
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 -->

This is useful when running our doctester in parallel with other build
steps, or several doctesters in parallel, as happens for example in
`make SAGE_CHECK=yes pypi-wheels`, and more of that after sagemath#35095.

To test:
```
MAKE="make -j14" make SAGE_NUM_THREADS=100 DEBUG_JOBCLIENT=1 ptest
```
This will make the doctester attempt to use 100 workers, but it will
only get tokens for 14 workers from `make`.
`DEBUG_JOBCLIENT=1` shows what's happening.

Upstream PR:
- milahu/gnumake-tokenpool#3 (merged)

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
- Resolves sagemath#30369
<!-- 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.
- [ ] 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
- 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#36640
Reported by: Matthias Köppe
Reviewer(s): Michael Orlitzky
@vbraun vbraun merged commit 77fbf9a into sagemath:develop Dec 14, 2023
@jhpalmieri
Copy link
Copy Markdown
Member

jhpalmieri commented Dec 18, 2023

This PR seems to be causing alarm problems on OS X, as reported at https://groups.google.com/g/sage-release/c/uP-rwlM__MU/m/MuI_BhfmCQAJ. It may be fine on older versions of OS X, according to someone else's experience.

@mkoeppe
Copy link
Copy Markdown
Contributor Author

mkoeppe commented Dec 18, 2023

How to reproduce these failures? Do they show with sage -tp or only with make ptest?

@mkoeppe
Copy link
Copy Markdown
Contributor Author

mkoeppe commented Dec 18, 2023

The AssertionError that you report in https://groups.google.com/g/sage-release/c/uP-rwlM__MU/m/IXLL5mZrAwAJ seems unrelated; I would think if this is new, it's coming from one of the recent IPython upgrade PRs

@jhpalmieri
Copy link
Copy Markdown
Member

jhpalmieri commented Dec 18, 2023

How to reproduce these failures? Do they show with sage -tp or only with make ptest?

I've only seen them with make ptestlong. I've been unable to reproduce them with sage -tp ....

@jhpalmieri
Copy link
Copy Markdown
Member

jhpalmieri commented Dec 18, 2023

The AssertionError that you report in https://groups.google.com/g/sage-release/c/uP-rwlM__MU/m/IXLL5mZrAwAJ seems unrelated; I would think if this is new, it's coming from one of the recent IPython upgrade PRs

If I remember correctly, I also see this in 10.2.beta1, so it's not related to this PR. I don't know how long it's been happening. (I also see it on a linux virtual machine running on my Intel mac, so it's not just OS X.)

@mkoeppe
Copy link
Copy Markdown
Contributor Author

mkoeppe commented Dec 18, 2023

It may be related to #33428

@jhpalmieri
Copy link
Copy Markdown
Member

jhpalmieri commented Dec 21, 2023

I consider these alarm doctest failures to be a blocker issue. Aside from reverting this PR, what options are there?

@mkoeppe
Copy link
Copy Markdown
Contributor Author

mkoeppe commented Dec 21, 2023

Could you open a new Issue please with a clear reproducer (and distinguishing between ./sage -t and make test and make ptest)?

@jhpalmieri jhpalmieri mentioned this pull request Dec 21, 2023
Closed
2 tasks
@jhpalmieri
Copy link
Copy Markdown
Member

jhpalmieri commented Dec 21, 2023

Could you open a new Issue please with a clear reproducer (and distinguishing between ./sage -t and make test and make ptest)?

See #36944.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@orlitzky orlitzky orlitzky approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mkoeppe mkoeppe

Labels

c: build c: doctest framework

Projects

None yet

Milestone

sage-10.3

Development

Successfully merging this pull request may close these issues.

Parallel docbuild, cythonization: Use GNU make's POSIX jobserver protocol

4 participants

@mkoeppe @orlitzky @vbraun @jhpalmieri