CPU and Max RSS Analysis tools #6663

ChrisPaulBennett · 2025-03-12T09:14:38Z

This apart of 3 pull requests for adding CPU time and Max RSS analysis to the Cylc UI.

This adds the Max RSS and CPU time (as measured by cgroups) to the table view, box plot and time series views.

This adds a python profiler script. This profiler will will be ran by cylc in the same crgroup as the cylc task. It will periodically poll cgroups and save data to a file. Cylc will then store these values in the sql db file.

Linked to;
cylc/cylc-ui#2100
cylc/cylc-uiserver#675

Check List

I have read CONTRIBUTING.md and added my name as a Code Contributor.
Contains logically grouped changes (else tidy your branch by rebase).
Does not contain off-topic changes (use other PRs for other changes).
Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
Tests are included (or explain why tests are not needed).
Changelog entry included if this is a change that can affect users
Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

oliver-sanders

🎉

cylc/flow/etc/job.sh

cylc/flow/job_file.py

oliver-sanders · 2025-03-12T09:57:11Z

cylc/flow/etc/job.sh

@@ -138,6 +138,10 @@ cylc__job__main() {
    mkdir -p "$(dirname "${CYLC_TASK_WORK_DIR}")" || true
    mkdir -p "${CYLC_TASK_WORK_DIR}"
    cd "${CYLC_TASK_WORK_DIR}"
+
+    cylc profile &


TODO: Pass through a value for the -m option.

oliver-sanders · 2025-03-12T10:07:59Z

cylc/flow/scripts/profile.py

+    # Get the PID of the current process
+    pid = os.getpid()
+    # Get the cgroup information for the current process
+    result = subprocess.run(['cat', '/proc/' + str(pid) + '/cgroup'], capture_output=True, text=True)


Look at using pathlib.Path for filesystem path building / manipulation.

I think capture_output captures both stdout and stderr, so if there is any error here, it will be swallowed. You might want to only capture stdout (stdout=subprocess.PIPE) or report stderr if the process fails (print(result.stderr, file=sys.stderr)).

oliver-sanders · 2025-03-12T10:11:38Z

cylc/flow/scripts/profile.py

+            print(e)
+            raise FileNotFoundError("cgroups not found:" + args.cgroup_location)


Probs best to write a user-friendly message to stderr, then raise the original exception, e.g:

Suggested change

print(e)

raise FileNotFoundError("cgroups not found:" + args.cgroup_location)

print(f'cgroups not found: {args.cgroup_location}', file=sys.stderr)

raise from None

oliver-sanders · 2025-03-12T10:11:49Z

cylc/flow/scripts/profile.py

+        except FileNotFoundError as e:
+            print(e)
+            raise FileNotFoundError("cgroups not found:" + args.cgroup_location)


oliver-sanders · 2025-03-12T10:13:52Z

cylc/flow/scripts/profile.py

+            except (OSError, IOError, ValueError) as error:
+                print(error)


(should write the error to stderr not stdout)

I think this is here to tolerate failures in cgroup polling? I guess this might happen from time to time?

Perhaps the profiler should bail if, say, three polls fail in a row (indicating a systematic failure) rather than continuing with fruitless polling?

tests/functional/jobscript/02-profiler.t

Changed the name of the profiler module. Linting Profiler sends KB instead of bytes Time Series now working CPU/Memory Logging working

Initial profiler implementation (non working) Changed the name of the profiler module. Linting Profiler sends KB instead of bytes Time Series now working CPU/Memory Logging working Adding profiler unit tests updating tests Fail gracefully if cgroups cannot be found Revert "Fail gracefully if cgroups cannot be found" This reverts commit 92e1e11c9b392b4742501d399f191f590814e95e. Linting Modifying unit tests Linting Changed the name of the profiler module. Profiler sends KB instead of bytes Time Series now working

Co-authored-by: Hilary James Oliver <[email protected]>

* The COPYING file appears to have moved from `dist-info/COPYING` into `dist-info/licenses/COPYING`.

Co-authored-by: Hilary James Oliver <[email protected]>

* Attempt to fix flaky test. * Cut out shell profile files to omit some spurious stderr.

Co-authored-by: Hilary James Oliver <[email protected]>

* Attempt to fix flaky test. * Cut out shell profile files to omit some spurious stderr.

Revert "Fail gracefully if cgroups cannot be found" This reverts commit 92e1e11c9b392b4742501d399f191f590814e95e. Adding profiler unit tests updating tests

* Attempt to fix flaky test. * Cut out shell profile files to omit some spurious stderr.

…rofiler

oliver-sanders

👍

oliver-sanders · 2025-04-03T10:37:01Z

cylc/flow/cfgspec/globalcfg.py

+
+                .. versionadded:: 8.0.0
+                ''')
+                Conf('cgroups path', VDR.V_STRING, '/sys/fs/cgroup')


Please add a description for this configuration.

oliver-sanders · 2025-04-03T10:39:47Z

cylc/flow/etc/job.sh

@@ -157,11 +163,23 @@ cylc__job__main() {
            cylc__set_return "$ret_code"
        fi
    }
+    # Grab the max rss and cpu_time value before moving directory
+    if [[ -f "max_rss" ]]; then
+      max_rss=$(sed -n '1p' max_rss)


The max_rss file should only have a single value in it, so we shouldn't need to extract only the first line.

oliver-sanders · 2025-04-03T10:39:57Z

cylc/flow/etc/job.sh

+      rm max_rss
+    fi
+    if [[ -f "cpu_time" ]]; then
+      cpu_time=$(sed -n '1p' cpu_time)


oliver-sanders · 2025-04-03T10:42:15Z

cylc/flow/scripts/profiler.py

+        default=10, dest="delay")
+    parser.add_option(
+        "-m", type=str, help="Location of cgroups directory",
+        default="/sys/fs/cgroup",


Since this default is hardcoded in the Cylc config, we could make this a required argument to avoid the need to hardcode this default twice.

oliver-sanders · 2025-04-03T10:52:09Z

cylc/flow/scripts/profiler.py

+def write_data(data, filename):
+    with open(filename, 'w') as f:
+        f.write(data + "\n")


This is adding one line to the file for each poll. The Cylc job script picks the first line of the file and sends it back to the scheduler. So I think we're sending back the first poll of the memory usage, not the max poll?

We only need to store the maximum values in these files, so we could do the maths in Python and keep this file to a single line:

with open('max_rss', 'w+') as max_rss: prev_memory = 0 while keep_looping(): memory = poll_memory() if memory > prev_memory: max_rss.seek(0, 0) # wind back to the start of the file max_rss.write(f'{memory}\n') # (over)write the first line max_rss.flush() prev_memory = memory time.sleep(delay)

oliver-sanders · 2025-04-03T10:59:35Z

tests/functional/jobscript/02-profiler.t

+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#-------------------------------------------------------------------------------
+# cylc profile test


This test will run regular background jobs, no slurm / pbs / whatever, so no cgroups.

I think this is testing that the profiler will not cause the job to fail, even if it cannot poll cgroups? Which is worthwhile testing.

We should test the jobs stderr for the line(s) written by the profiler script complaining of the fault.

oliver-sanders · 2025-04-03T11:00:37Z

tests/functional/jobscript/02-profiler/flow.cylc

+[meta]
+    title = "job script torture test"
+
+    description = """Any task job script may fail regardless of user runtime
+settings if changes to cylc re-order the job script sections badly: e.g.
+"cylc task started" must be called after the CYLC_ environment variables
+are exported. Additionally, users may rely on the order of variable
+definition in each environment and script section: e.g. workflow
+bin directory must go in PATH before the task runtime environment is
+defined because workflow bin commands could be used in variable assignment
+expressions."""


I'm not sure this is relevant here?

oliver-sanders · 2025-04-03T11:00:50Z

tests/functional/jobscript/02-profiler/flow.cylc

+echo "HELLO FROM INIT-SCRIPT"
+# define a variable
+export VAR_IS=is"""
+        pre-script = """
+echo "HELLO FROM PRE-SCRIPT"
+# init-script must be done:
+echo VAR_IS is $VAR_IS
+# user environment must be done:


Needed for this test?

oliver-sanders · 2025-04-03T11:02:32Z

cylc/flow/scripts/profiler.py

+def get_config(args):
+    # Find the cgroup that this process is running in.
+    # Cylc will put this profiler in the same cgroup
+    # as the job it is profiling
+    cgroup_name = get_cgroup_name()
+    cgroup_version = get_cgroup_version(args.cgroup_location, cgroup_name)
+    process = get_cgroup_paths(cgroup_version,
+                               args.cgroups_location,
+                               cgroup_name)
+
+    profile(process, cgroup_version, args.delay)


Codecov has spotted a couple of test coverage holes here that it would be good to fill.

oliver-sanders · 2025-04-03T11:03:48Z

(please ignore the manylinux test failures, we'll be removing this test on master shortly)

ChrisPaulBennett marked this pull request as draft March 12, 2025 09:19

oliver-sanders reviewed Mar 12, 2025

View reviewed changes

oliver-sanders added this to the 8.x milestone Mar 12, 2025

oliver-sanders assigned ChrisPaulBennett Mar 12, 2025

This was referenced Mar 13, 2025

CPU and Max RSS Analysis tools cylc/cylc-ui#2100

Open

CPU and Max RSS Analysis tools cylc/cylc-uiserver#675

Open

ChrisPaulBennett force-pushed the cylc_profiler branch 2 times, most recently from fb1b12b to c5d30b3 Compare March 21, 2025 11:37

Initial profiler implementation (non working)

246a4c1

Changed the name of the profiler module. Linting Profiler sends KB instead of bytes Time Series now working CPU/Memory Logging working

ChrisPaulBennett force-pushed the cylc_profiler branch 2 times, most recently from 4a6dbe8 to 30a7bb0 Compare April 1, 2025 15:31

ChrisPaulBennett and others added 19 commits April 2, 2025 09:34

GH Actions: use explicit bash shell & other defaults

171f7ee

host-select: fix compatibility with force-condemned hosts

7d50d0b

Update cylc/flow/cfgspec/globalcfg.py

8608088

Co-authored-by: Hilary James Oliver <[email protected]>

actions: update build action to support python 3.13

060056b

* The COPYING file appears to have moved from `dist-info/COPYING` into `dist-info/licenses/COPYING`.

host-select: fix compatibility with force-condemned hosts

a0cdcd6

Update cylc/flow/cfgspec/globalcfg.py

3d4a187

Co-authored-by: Hilary James Oliver <[email protected]>

tests: convert unittest to pytest

3057edd

tests/u: test_subprocpool.py::test_run_command_writes_to_err

a6ecd29

* Attempt to fix flaky test. * Cut out shell profile files to omit some spurious stderr.

Time Series now working

e5119b4

Profiler sends KB instead of bytes

9f593fc

Modifying unit tests

5f84c2f

host-select: fix compatibility with force-condemned hosts

47a4a98

Update cylc/flow/cfgspec/globalcfg.py

356abff

Co-authored-by: Hilary James Oliver <[email protected]>

tests: convert unittest to pytest

6ed1770

tests/u: test_subprocpool.py::test_run_command_writes_to_err

4cfcffe

* Attempt to fix flaky test. * Cut out shell profile files to omit some spurious stderr.

Linting

1e5b804

Fail gracefully if cgroups cannot be found

38c31f5

Revert "Fail gracefully if cgroups cannot be found" This reverts commit 92e1e11c9b392b4742501d399f191f590814e95e. Adding profiler unit tests updating tests

tests: convert unittest to pytest

339af9c

oliver-sanders and others added 3 commits April 2, 2025 09:34

tests/u: test_subprocpool.py::test_run_command_writes_to_err

391559b

* Attempt to fix flaky test. * Cut out shell profile files to omit some spurious stderr.

Adding profiler unit tests

b9c080c

adding pycharm files to .gitignore file

7091711

ChrisPaulBennett force-pushed the cylc_profiler branch from 30a7bb0 to 7091711 Compare April 2, 2025 08:35

ChrisPaulBennett added 6 commits April 2, 2025 10:07

Fixing my terrible rebasing

ad6b3a1

Merge remote-tracking branch 'refs/remotes/origin/master' into cylc_p…

5bddeef

…rofiler

MyPy Linting

afcdd81

More unit tests

378dcb9

Linting

ca1e796

Adding towncrier fragment

58fae4f

ChrisPaulBennett marked this pull request as ready for review April 2, 2025 14:20

oliver-sanders reviewed Apr 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CPU and Max RSS Analysis tools #6663

CPU and Max RSS Analysis tools #6663

ChrisPaulBennett commented Mar 12, 2025 •

edited

Loading

oliver-sanders left a comment

oliver-sanders Mar 12, 2025

oliver-sanders Mar 12, 2025

oliver-sanders Mar 12, 2025

oliver-sanders Mar 12, 2025

oliver-sanders Mar 12, 2025

oliver-sanders left a comment

oliver-sanders Apr 3, 2025

oliver-sanders Apr 3, 2025

oliver-sanders Apr 3, 2025

oliver-sanders Apr 3, 2025

oliver-sanders Apr 3, 2025

oliver-sanders Apr 3, 2025

oliver-sanders Apr 3, 2025

oliver-sanders Apr 3, 2025

oliver-sanders Apr 3, 2025

oliver-sanders commented Apr 3, 2025

		print(e)
		raise FileNotFoundError("cgroups not found:" + args.cgroup_location)

CPU and Max RSS Analysis tools #6663

Are you sure you want to change the base?

CPU and Max RSS Analysis tools #6663

Conversation

ChrisPaulBennett commented Mar 12, 2025 • edited Loading

oliver-sanders 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

oliver-sanders 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

oliver-sanders commented Apr 3, 2025

ChrisPaulBennett commented Mar 12, 2025 •

edited

Loading