Time series logging #96

matthiasdiener · 2020-09-11T02:24:26Z

Implements https://github.com/illinois-ceesd/planning/issues/57

matthiasdiener · 2020-09-11T12:58:50Z

Here is how the output looks like (x-axis is simtime, y-axis is walltime per step):

matthiasdiener · 2020-09-30T02:07:44Z

@inducer Is this going in the right direction (in particular examples/sod-mpi.py and mirgecom/steppers.py)?

Edit:
I'm thinking of doing the following:

Putting the LogQuantity classes into their own file, mirgecom/logging.py or so.
Adding the startup code to the mpi_entry_point decorator.

mirgecom/steppers.py

inducer · 2020-10-01T15:46:25Z

mirgecom/steppers.py

+
+  def __call__(self):
+    from functools import partial
+    _min = partial(self.discr.nodal_min, "vol")


Make sure that there's no MPI reduction in this.

As far as I can see, there is none.

mirgecom/steppers.py

matthiasdiener · 2021-02-22T18:22:57Z

I fixed most of the comments that you had @inducer. For two things I will need some more guidance:

the delete issue
the kernel results traversal

inducer · 2021-02-22T23:48:21Z

For two things I will need some more guidance:

Commented inline.

inducer · 2021-02-23T15:31:47Z

To cut down on noise in my email, I'm unsubscribing from this PR. When it next needs my attention, please @-mention me or hit the "request review" button. Otherwise, I may not see your messages in a timely manner.

matthiasdiener · 2021-02-24T21:33:13Z

I think I addressed all your comments @inducer. Ready for another review.

inducer

Alright, getting close. These three, plus the doc CI failure, and then this is good to go IMO.

mirgecom/profiling.py

inducer · 2021-02-24T22:02:27Z

mirgecom/profiling.py

@@ -253,7 +278,7 @@ def tabulate_profiling_data(self) -> pytools.Table:
                bandwidth_access_mean = "--"
                bandwidth_access_max = "--"


This code could probably just use get_profiling_data_for_kernel now, to avoid code duplication.

I'm not sure this would be too useful. get_profiling_data_for_kernel only returns averages of some statistics, while tabulate_profiling_data also calculates min/max as well as derived statistics such as bandwidth etc. Imho we wouldn't gain much by using get_profiling_data_for_kernel here.

I mean, that's kind of a strange inconsistency, too. Why can one gather max/min, the other not?

We can modify get_profiling_data_for_kernel to also return (and log) min/max, if you prefer.

How about returning something like this:

class StatisticsAccumulator: def __init__(self): self.num_values = 0 self._sum = 0 self._min = 0 self._max = 0 def add_value(self, v): if v is None: return self.num_values += 1 self._sum += v # ... def mean(self): if self.num_values == 0: return None return self.sum / self.num_values # ...

Just as a side note, I think my original idea for just doing averages here was to reduce the sheer number of statistics and the associated clutter (+overhead). Just adding min/max would triple the amount of kernel profiling data, which is already by far the majority of data stored for each time step. Also, it would create "nice"-looking entries like multiply_time_min.max.

Sure! I'm not saying we have to create LogQuantitys for all of them. This accumulator thing is just so that we can unify the summation logic.

Added the solution you outlined. This works a bit differently for the derived quantities

mirgecom/profiling.py

inducer · 2021-02-24T22:08:08Z

mirgecom/logging_quantities.py

+    logmgr.set_constant("cl_device_name", str(queue.device))
+
+
+def logmgr_add_default_discretization_quantities(logmgr: LogManager, discr, dim,


Suggested change

def logmgr_add_default_discretization_quantities(logmgr: LogManager, discr, dim,

def logmgr_add_all_discretization_quantities(logmgr: LogManager, discr, dim,

The quantities added here aren't necessarily all discretization-related quantities, so renaming this is maybe confusing.

Agreed. add_many?

Could be, or add_basic?

Don't like add_basic so much.

renamed to add_many.

Co-authored-by: Andreas Klöckner <[email protected]>

matthiasdiener · 2021-02-25T03:18:02Z

Ready for another review @inducer.

inducer

Super close. Just a few tweaks to make StatisticsAccumulator general purpose.

inducer · 2021-02-25T04:24:26Z

mirgecom/profiling.py

+        self._min = sys.maxsize
+        self._max = 0


I think it's better to start with None, to avoid assuming bounds.

Doesn't look done. Forgot to push?

Yep, sorry. Should be pushed now.

inducer · 2021-02-25T04:24:48Z

mirgecom/profiling.py

+    """Class that provides statistical functions for profile results.
+
+    Values are converted to "Giga".
+    """


Add .. automethod for methods.

Document num_values.

Doesn't look done. Forgot to push?

inducer · 2021-02-25T04:25:37Z

mirgecom/profiling.py

@@ -51,15 +52,68 @@ class SingleCallKernelProfile:
    footprint_bytes: int


+class StatisticsAccumulator:
+    """Class that provides statistical functions for profile results.


No real reason to me to make this special-purpose. Move to mirgecom.tools?

Moved to mirgecom.utils (and pushed).

inducer · 2021-02-25T04:26:07Z

mirgecom/profiling.py

+class StatisticsAccumulator:
+    """Class that provides statistical functions for profile results.
+
+    Values are converted to "Giga".


Why not introduce a scale_factor constructor parameter (defaulting to 1) so as to not wreck the general-purpose-ness?

Doesn't look done. Forgot to push?

matthiasdiener · 2021-02-25T14:26:50Z

Should be ready for another review @inducer.

initial logging

88daa08

matthiasdiener self-assigned this Sep 11, 2020

flake8

9eba876

matthiasdiener changed the title ~~initial logging~~ initial time series logging Sep 15, 2020

matthiasdiener added 3 commits September 29, 2020 13:29

Merge branch 'master' into logging

6cf0c54

add logpyle

e966864

use new interface for sod-mpi

bc96f56

matthiasdiener commented Sep 30, 2020

View reviewed changes

mirgecom/steppers.py Outdated Show resolved Hide resolved

updates

58e6bde

inducer reviewed Oct 1, 2020

View reviewed changes

inducer mentioned this pull request Oct 1, 2020

Automatically label plots illinois-ceesd/logpyle#16

Closed

inducer reviewed Oct 1, 2020

View reviewed changes

mirgecom/steppers.py Outdated Show resolved Hide resolved

mirgecom/steppers.py Outdated Show resolved Hide resolved

matthiasdiener added 2 commits October 5, 2020 10:50

callback

e614232

flake8

0b8edf0

matthiasdiener changed the title ~~initial time series logging~~ Time series logging Oct 5, 2020

matthiasdiener requested a review from inducer October 5, 2020 15:55

matthiasdiener added 12 commits October 5, 2020 10:57

fix wave eager

b726541

Merge branch 'master' into logging

aedbd05

add kernel profile

8995505

better names

83b533d

unique

a468bc9

cleanups

ef278dd

cleanups 2

b8e3b2e

Merge branch 'master' into logging

6777a01

clean state passing

13c6401

Merge remote-tracking branch 'origin/master' into logging

9574ded

fix order

0237e58

add comments

1985888

matthiasdiener added 2 commits February 22, 2021 11:29

remove wait_for_events arg

7eea01b

rename key, value

a6492cd

key value rename

785efc0

MTCam mentioned this pull request Feb 23, 2021

Checkpoint only shows temperature/pressure from rank 0 #258

Closed

matthiasdiener added 3 commits February 23, 2021 10:54

fix+comments

a457741

index profile_results by kernel name

8ea0a61

separate get and reset

305b658

inducer reviewed Feb 24, 2021

View reviewed changes

matthiasdiener and others added 6 commits February 24, 2021 16:15

fix doc

3beff13

Apply suggestions from code review

b0a2d3e

Co-authored-by: Andreas Klöckner <[email protected]>

fix fail

5c74dc7

be more explicit about averages

0a0b216

rename logmgr_add_many_discretization_quantities

1a14442

add StatisticsAccumulator

16962c2

inducer reviewed Feb 25, 2021

View reviewed changes

matthiasdiener added 2 commits February 25, 2021 08:17

fix Statistics

99e56e3

move StatisticsAccumulator to mirgecom.utils

f23b1c8

matthiasdiener added 2 commits February 25, 2021 08:48

type annotations

0ec5294

change scale_factor to multiply

944d5c9

inducer approved these changes Feb 25, 2021

View reviewed changes

matthiasdiener enabled auto-merge (squash) February 25, 2021 16:39

matthiasdiener merged commit bbaf0dc into master Feb 25, 2021

matthiasdiener deleted the logging branch February 25, 2021 17:40

matthiasdiener mentioned this pull request Sep 13, 2023

Add version information to logging subsystem #116

Closed

		@@ -253,7 +278,7 @@ def tabulate_profiling_data(self) -> pytools.Table:
		bandwidth_access_mean = "--"
		bandwidth_access_max = "--"

		logmgr.set_constant("cl_device_name", str(queue.device))


		def logmgr_add_default_discretization_quantities(logmgr: LogManager, discr, dim,

	def logmgr_add_default_discretization_quantities(logmgr: LogManager, discr, dim,
	def logmgr_add_all_discretization_quantities(logmgr: LogManager, discr, dim,

Time series logging #96

Time series logging #96

Conversation

matthiasdiener commented Sep 11, 2020 • edited Loading

matthiasdiener commented Sep 11, 2020

matthiasdiener commented Sep 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasdiener commented Feb 22, 2021

inducer commented Feb 22, 2021

inducer commented Feb 23, 2021

matthiasdiener commented Feb 24, 2021

inducer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasdiener Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasdiener Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasdiener Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasdiener Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasdiener commented Feb 25, 2021

inducer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasdiener commented Feb 25, 2021

matthiasdiener commented Sep 11, 2020 •

edited

Loading

matthiasdiener commented Sep 30, 2020 •

edited

Loading

matthiasdiener Feb 24, 2021 •

edited

Loading

matthiasdiener Feb 24, 2021 •

edited

Loading

matthiasdiener Feb 24, 2021 •

edited

Loading

matthiasdiener Feb 24, 2021 •

edited

Loading