#23892 closed defect (fixed)
Run doctests with OMP_NUM_THREADS=2
Reported by: | jdemeyer | Owned by: | |
---|---|---|---|
Priority: | blocker | Milestone: | sage-8.1 |
Component: | packages: optional | Keywords: | |
Cc: | mderickx | Merged in: | |
Authors: | Jeroen Demeyer | Reviewers: | Maarten Derickx |
Report Upstream: | N/A | Work issues: | |
Branch: | 9f9f7b7 (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description (last modified by )
The normaliz
package uses OMP for threading, which can create many threads. In doctests, this is bad for two reasons:
- Doctests should not use an unexpectedly large number of system resources.
- When there are too many threads, the virtual memory limit from #23748 will be hit.
There is a solution: set the environment variable OMP_NUM_THREADS=2
while doctesting.
Change History (21)
comment:1 Changed 5 years ago by
comment:2 Changed 5 years ago by
- Cc mderickx added
comment:3 Changed 5 years ago by
- Description modified (diff)
comment:4 Changed 5 years ago by
- Description modified (diff)
comment:5 Changed 5 years ago by
- Description modified (diff)
- Summary changed from Various doctest failures if pynormaliz is installed to Run doctests with OMP_NUM_THREADS=2
comment:6 Changed 5 years ago by
- Branch set to u/jdemeyer/run_doctests_with_omp_num_threads_2
comment:7 Changed 5 years ago by
- Commit set to 9f9f7b71f56c1c7ac20ed8601355ef495bf42fa3
- Status changed from new to needs_review
New commits:
9f9f7b7 | Run doctests with OMP_NUM_THREADS=2
|
comment:8 follow-up: ↓ 10 Changed 5 years ago by
Hi Jeroen,
I want to review this, but before doing so I run into trouble, because on my machine all doctests pass in sage 8.1.beta5 with pynormaliz. Could you give pointers to which doctests failed for you and maybe help reproduce the failure so I can better understand wether this solution works.
comment:9 Changed 5 years ago by
In particular, many tests in src/sage/combinat/rigged_configurations
fail for me with pynormaliz
.
Since the problem depends on the number of threads, which is the number of cores by default, it could very well be that this problem only occurs on systems with many cores. The system where I saw the failure has 24 cores. Maybe you could get the failure with OMP_NUM_THREADS=24
?
comment:10 in reply to: ↑ 8 Changed 5 years ago by
Replying to mderickx:
Also is there any particular reason for the integer 2?
Yes, it is the smallest integer strictly larger than 1.
1 thread is too few, because it doesn't really test threading. With 2 threads, you do test threading. On the other hand, the system load will at most be a factor 200% too large, which is not too bad.
comment:11 follow-up: ↓ 12 Changed 5 years ago by
Without the patch I indeed get 4 files with failing doctests if I do:
export OMP_NUM_THREADS=24 sage -t long src/sage/combinat/rigged_configurations
and it does not fail anymore with the patch. So it seems to be the right thing to do in order to fix it.
One thing that I don't like about the current patch is that it overwrites OMP_NUM_THREADS
even if it is already explicitly set before running sage tests. This means that if someone for some reason wants to run the doctests with a different number of OMP_NUM_THREADS
for debugging purposes that involve problems with threading then one has to modify the source code. So I think it would be better to only set OMP_NUM_THREADS=2
if nothing was set before, providing a sane default value, but still allowing a less sane default value if one really insists. What are your thoughts on this?
comment:12 in reply to: ↑ 11 Changed 5 years ago by
Replying to mderickx:
One thing that I don't like about the current patch is that it overwrites
OMP_NUM_THREADS
even if it is already explicitly set before running sage tests.
I consider that a feature. The point is that doctests should be reproducible and not depend too much on the external environment. If somebody has set an environment variable OMP_NUM_THREADS
, the most likely reason is that he wants to use that number of threads for actual computations. It does not mean that he wants to use that many threads for doctests too.
This means that if someone for some reason wants to run the doctests with a different number of
OMP_NUM_THREADS
for debugging purposes that involve problems with threading then one has to modify the source code.
Alternatively, you can set os.environ['OMP_NUM_THREADS']
in a doctest too. That's easy to do and would fix the testing problem.
comment:13 follow-up: ↓ 14 Changed 5 years ago by
Ok, I am now running all the doctest after export OMP_NUM_THREADS=100
since I think standard patchbot testing is not good enough. If this passes then I will give positive review.
Does your remark mean that it would also be better to fix #23612 (edit: sorry I meant #23613) by unsetting PYTHONPATH
instead of making the doctest more admissible?
comment:14 in reply to: ↑ 13 Changed 5 years ago by
comment:15 Changed 5 years ago by
Sorry, I meant #23613.
comment:16 Changed 5 years ago by
- Status changed from needs_review to positive_review
Ok looks good to me.
comment:18 Changed 5 years ago by
- Reviewers set to Maarten Derickx
- Status changed from needs_work to positive_review
comment:19 Changed 5 years ago by
- Branch changed from u/jdemeyer/run_doctests_with_omp_num_threads_2 to 9f9f7b71f56c1c7ac20ed8601355ef495bf42fa3
- Resolution set to fixed
- Status changed from positive_review to closed
comment:20 Changed 5 years ago by
- Commit 9f9f7b71f56c1c7ac20ed8601355ef495bf42fa3 deleted
I wonder if this and/or #23748 will fix the doc build problems I was having on Windows for the past several weeks (which caused me to have to take down the Window patchbot >_<). Fingers crossed...
comment:21 Changed 5 years ago by
Oh wait, this was just for the doctests, I misread. Maybe not then...
Again??? :(