RadioAstronomySoftwareGroup / pyuvsim

A ultra-high precision package for simulating radio interferometers in python on compute clusters.
https://pyuvsim.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
43 stars 7 forks source link

Progbar update #345

Closed mkolopanis closed 3 years ago

mkolopanis commented 3 years ago

Keep writing progsteps to terminal after rank 0 finishes work

Description

Changed to a non-blocking barrier after the work execution loop. Registered a sys.stdout.close for non rank 0 processes. Newer openmpi versions complain about an open file handler. I could probably add a time.sleep() to the end of the loop for updating progess too, otherwise the rank 0 will just be writing a ton. maybe a few seconds?

Motivation and Context

closes #338 This only resolves the actual issue of the progsteps stopping writing. It does not address the delay we were investigating in that issue. I think we should probably make the delay thing a new issue anyway.

Types of changes

Checklist:

For all pull requests:

Bug fix checklist:

codecov[bot] commented 3 years ago

Codecov Report

Merging #345 (6a6ed67) into main (6971648) will increase coverage by 0.00%. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #345   +/-   ##
=======================================
  Coverage   99.01%   99.02%           
=======================================
  Files          13       13           
  Lines        1931     1945   +14     
=======================================
+ Hits         1912     1926   +14     
  Misses         19       19           
Impacted Files Coverage Δ
pyuvsim/mpi.py 100.00% <ø> (ø)
pyuvsim/uvsim.py 98.46% <100.00%> (+0.06%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6971648...6a6ed67. Read the comment docs.

mkolopanis commented 3 years ago

@aelanman what are your thoughts about adding a time.sleep to the end of the updating loop?

aelanman commented 3 years ago

It shouldn't be necessary for the progsteps, because it only prints when the counter value has stepped by 1% of the total number of tasks (or for every step if there are fewer than 100 tasks). However, it would reduce the number of calls to counter.current_value()... I'm not sure if having rank 0 constantly locking and unlocking the counter would slow down other processes.

mkolopanis commented 3 years ago

Are we interested in print statements to show time to complete some parts like these? image

aelanman commented 3 years ago

That looks like a good idea. You should probably include " min" at the end of the format strings so the user knows the time is measured in minutes.

mkolopanis commented 3 years ago

Time is an astropy object (or subclass if from the moon repo right?) it should print out the units too.

aelanman commented 3 years ago

Oh right, of course. Looks good!

mkolopanis commented 3 years ago

@aelanman I think this is ready for another look.