Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#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) Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

The normaliz package uses OMP for threading, which can create many threads. In doctests, this is bad for two reasons:

  1. Doctests should not use an unexpectedly large number of system resources.
  1. 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 2 years ago by mderickx

Again??? :(

comment:2 Changed 2 years ago by mderickx

  • Cc mderickx added

comment:3 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:4 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:5 Changed 2 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Description modified (diff)
  • Summary changed from Various doctest failures if pynormaliz is installed to Run doctests with OMP_NUM_THREADS=2

comment:6 Changed 2 years ago by jdemeyer

  • Branch set to u/jdemeyer/run_doctests_with_omp_num_threads_2

comment:7 Changed 2 years ago by jdemeyer

  • Commit set to 9f9f7b71f56c1c7ac20ed8601355ef495bf42fa3
  • Status changed from new to needs_review

New commits:

9f9f7b7Run doctests with OMP_NUM_THREADS=2

comment:8 follow-up: Changed 2 years ago by mderickx

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. Also is there any particular reason for the integer 2? Why not 3 or 4? I agree it should not be 1 because certain bugs might go undetected in that way.

Last edited 2 years ago by mderickx (previous) (diff)

comment:9 Changed 2 years ago by jdemeyer

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 2 years ago by jdemeyer

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: Changed 2 years ago by mderickx

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 2 years ago by jdemeyer

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: Changed 2 years ago by mderickx

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?

Last edited 2 years ago by mderickx (previous) (diff)

comment:14 in reply to: ↑ 13 Changed 2 years ago by jdemeyer

Replying to mderickx:

Does your remark mean that it would also be better to fix #23612 by unsetting PYTHONPATH instead of making the doctest more admissible?

Are you sure you mean #23612? It seems unrelated to PYTHONPATH or doctests.

comment:15 Changed 2 years ago by mderickx

Sorry, I meant #23613.

comment:16 Changed 2 years ago by mderickx

  • Status changed from needs_review to positive_review

Ok looks good to me.

comment:17 Changed 2 years ago by vbraun

  • Status changed from positive_review to needs_work

Reviewer name

comment:18 Changed 2 years ago by mderickx

  • Reviewers set to Maarten Derickx
  • Status changed from needs_work to positive_review

comment:19 Changed 2 years ago by vbraun

  • 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 2 years ago by embray

  • 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 2 years ago by embray

Oh wait, this was just for the doctests, I misread. Maybe not then...

Note: See TracTickets for help on using tickets.