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

Printing messages in parallel runs #937

Merged
merged 3 commits into from
Jan 14, 2025

Conversation

jhdark
Copy link
Collaborator

@jhdark jhdark commented Jan 13, 2025

Proposed changes

Due to the way printing in python has been used within festim means that print messages are shown on every node, which in practice means printing the message repeatedly by the number of processors defined. For instance running the following script:

import festim as F
import numpy as np

my_model = F.Simulation(
    mesh=F.MeshFromVertices(vertices=np.linspace(0, 1, num=1000)),
    materials=F.Material(id=1, D_0=1, E_D=0),
    temperature=300,
    settings=F.Settings(
        absolute_tolerance=1e-10,
        relative_tolerance=1e-10,
        transient=False,
    ),
)
my_model.initialise()
my_model.run()

running this in parallel using mpirun -np 4 python mwe.py with 4 processors would produce the following:

Defining initial values
Defining variational problem
Defining initial values
Defining variational problem
Defining initial values
Defining variational problem
Defining initial values
Defining variational problem
Defining source terms
Defining source terms
Defining source terms
Defining source terms
Defining boundary conditions
Solving steady state problem...
Defining boundary conditions
Solving steady state problem...
Defining boundary conditions
Solving steady state problem...
Defining boundary conditions
Solving steady state problem...
Solved problem in 0.00 s
Solved problem in 0.00 s
Solved problem in 0.00 s
Solved problem in 0.00 s

With these changes, the printing message is only done on the master process (or rank 0 process). Meaning printed messages will only appear once, no matter the number of processors used.

resulting in:

Defining initial values
Defining variational problem
Defining source terms
Defining boundary conditions
Solving steady state problem...
Solved problem in 0.00 s

making printed messaged much cleaner and easier for users to understand.

@KulaginVladimir don't suppose you know of anyway we could test this? I imagine it may have to be at the github actions level in order to run the test scripts in parallel. Which itself could open up a new can of worms as we should really be testing in parallel too.

Types of changes

What types of changes does your code introduce to FESTIM?

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code refactoring
  • Documentation Update (if none of the other choices apply)
  • New tests

Checklist

  • Black formatted
  • Unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

Copy link

codecov bot commented Jan 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.27%. Comparing base (8d6857b) to head (3eef19e).
Report is 47 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #937   +/-   ##
=======================================
  Coverage   99.27%   99.27%           
=======================================
  Files          60       60           
  Lines        2753     2760    +7     
=======================================
+ Hits         2733     2740    +7     
  Misses         20       20           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@KulaginVladimir
Copy link
Collaborator

Hi @jhdark !

@KulaginVladimir don't suppose you know of anyway we could test this?

Sorry, but I don't. Maybe, we could search for a way somewhere?

By the way, all Error/Warning messages are printed by each node when running in parallel, no?

@jhdark
Copy link
Collaborator Author

jhdark commented Jan 13, 2025

Yes this is true, but error messages are exceptional cases, so might not be completely necessary.

@RemDelaporteMathurin
Copy link
Collaborator

I agree with @jhdark errors shouldn't occur in a normal situation so that is not worth rewriting eveyrthing.

@RemDelaporteMathurin RemDelaporteMathurin linked an issue Jan 14, 2025 that may be closed by this pull request
Copy link
Collaborator

@RemDelaporteMathurin RemDelaporteMathurin left a comment

Choose a reason for hiding this comment

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

@jhdark should we add a function

def festim_print(msg):
    if MPI.comm_word.rank == 0:
        print(msg)

@jhdark
Copy link
Collaborator Author

jhdark commented Jan 14, 2025

Can do, might make the script a little easier to read. And just apply it to the changes that have been made?

@RemDelaporteMathurin
Copy link
Collaborator

Can do, might make the script a little easier to read. And just apply it to the changes that have been made?

Yes

@jhdark jhdark merged commit b494df6 into festim-dev:main Jan 14, 2025
6 checks passed
@jhdark jhdark deleted the printing_in_parallel branch January 14, 2025 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicated info messages when running in parallel
3 participants