Skip to content

CPU and Max RSS Analysis tools - #675

Open
ChrisPaulBennett wants to merge 46 commits into
cylc:masterfrom
ChrisPaulBennett:memory_view
Open

ChrisPaulBennett wants to merge 46 commits into
cylc:masterfrom
ChrisPaulBennett:memory_view

Conversation

@ChrisPaulBennett

@ChrisPaulBennett ChrisPaulBennett commented Mar 12, 2025 •

Copy link
Copy Markdown
Contributor

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.

Linked to;
cylc/cylc-flow#6663
cylc/cylc-ui#2100

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.

@ChrisPaulBennett
ChrisPaulBennett marked this pull request as draft March 12, 2025 09:19
Comment thread cylc/uiserver/schema.py Resolved
@MetRonnie

This comment was marked as resolved.

@ChrisPaulBennett

Copy link
Copy Markdown
Contributor Author

I've merged in the previous changes to the SQL infrastructure that deals with task states. @oliver-sanders I've had to change some of your code that creates parts of the SQL query, mostly just to take into account the new join.
The codecov failures are from code that I haven't touched.

@MetRonnie

This comment was marked as resolved.

Comment thread cylc/uiserver/schema.py Resolved
Comment thread cylc/uiserver/schema.py Resolved
Comment thread cylc/uiserver/schema.py Resolved
Comment thread cylc/uiserver/schema.py Resolved
Comment thread cylc/uiserver/schema.py Resolved
Comment thread cylc/uiserver/schema.py Resolved

@MetRonnie MetRonnie left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These changes appear to have broken compatibility with workflows run in earlier versions of Cylc. I tested the tasks GraphQL query on a workflow that ran in 8.4.4.dev and got an empty result

query {
  tasks(live: false, workflows: ["wflow"]) {
    name
  }
}
{
  "data": {
    "tasks": []
  }
}

@ChrisPaulBennett

Copy link
Copy Markdown
Contributor Author

This been fixed. Max RSS and CPU time will be 0, if no data is present. An update to the cylc-ui needs to be made to not show cpu time / max rss charts if there is no data

@ChrisPaulBennett ChrisPaulBennett self-assigned this Jul 18, 2025
@MetRonnie MetRonnie added this to the 1.8.0 milestone Aug 4, 2025
@MetRonnie

Copy link
Copy Markdown
Member

This been fixed. Max RSS and CPU time will be 0, if no data is present. An update to the cylc-ui needs to be made to not show cpu time / max rss charts if there is no data

Is there a way to make this null instead of 0? Because currently the UI can't tell the difference between no data and times that are actually zero (to the nearest whole second).

@oliver-sanders oliver-sanders modified the milestones: 1.8.0, 1.9.0 Sep 17, 2025
@ChrisPaulBennett

Copy link
Copy Markdown
Contributor Author

This been fixed. Max RSS and CPU time will be 0, if no data is present. An update to the cylc-ui needs to be made to not show cpu time / max rss charts if there is no data

Is there a way to make this null instead of 0? Because currently the UI can't tell the difference between no data and times that are actually zero (to the nearest whole second).

This has been fixed in the UI repo

@MetRonnie

This comment was marked as resolved.

@oliver-sanders
oliver-sanders requested review from wxtim and removed request for MetRonnie January 13, 2026 15:38
@ChrisPaulBennett
ChrisPaulBennett marked this pull request as ready for review September 8, 2026 12:33

@oliver-sanders oliver-sanders left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some fields removed from the tests, not sure if this was purposeful?

Comment on lines -118 to -124
assert ret['started_time'] == '2022-12-14T15:01:00Z'
assert ret['state'] == 'succeeded'
assert ret['std_dev_queue_time'] == pytest.approx(0.0, 0.01)
assert ret['std_dev_run_time'] == pytest.approx(0.0, 0.01)
assert ret['std_dev_total_time'] == pytest.approx(0.0, 0.01)
assert ret['submit_num'] == 1
assert ret['submitted_time'] == '2022-12-14T15:00:00Z'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are we not testing started_time/submitted_time/state/submit_num any more?

Comment on lines 172 to -196
@@ -187,87 +249,115 @@
assert ret['min_total_time'] == 600
assert ret['name'] == 'Task_1'
assert ret['platform'] == 'MyPlatform'
assert ret['started_time'] == '2022-12-15T15:01:16Z'
assert ret['state'] == 'succeeded'
assert ret['std_dev_queue_time'] == pytest.approx(8.00, 0.01)
assert ret['std_dev_run_time'] == pytest.approx(52.0, 0.01)
assert ret['std_dev_total_time'] == pytest.approx(60.0, 0.01)
assert ret['submit_num'] == 1
assert ret['submitted_time'] == '2022-12-15T15:00:00Z'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are we not testing these fields any more?

Comment on lines 261 to -288
@@ -279,43 +369,29 @@
assert ret['min_total_time'] == 600
assert ret['name'] == 'Task_1'
assert ret['platform'] == 'MyPlatform'
assert ret['started_time'] == '2022-12-16T15:01:16Z'
assert ret['state'] == 'succeeded'
assert ret['std_dev_queue_time'] == pytest.approx(7.54, 0.01)
assert ret['std_dev_run_time'] == pytest.approx(49.02, 0.01)
assert ret['std_dev_total_time'] == pytest.approx(56.56, 0.01)
assert ret['submit_num'] == 1
assert ret['submitted_time'] == '2022-12-16T15:00:00Z'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are we not testing these fields any more?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No. They we aren't. These values are not apart of the task query anymore. I've also removed submit_num.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we've removed some of the fields that these functions used to return. E.g, id was returned before, but isn't now.

Note, this is unrelated to tidying unused parts of the SQL queries used within the functions.

Screenshot from 2026-09-24 11-15-44

Comment thread cylc/uiserver/schema.py Resolved
Comment thread cylc/uiserver/schema.py Resolved
Comment thread cylc/uiserver/schema.py Resolved
Comment thread cylc/uiserver/schema.py Resolved
Comment thread cylc/uiserver/schema.py Resolved
Comment thread cylc/uiserver/schema.py Resolved
Comment thread cylc/uiserver/schema.py Resolved
Comment thread cylc/uiserver/schema.py Resolved
@oliver-sanders oliver-sanders modified the milestones: 1.10.0, 1.11.0 Sep 30, 2026
Comment thread cylc/uiserver/schema.py Outdated
Comment on lines +131 to +133
cylc_version = CylcVersion(description=sstrip('''
Set the Cylc version that the workflow starts with.
''')
)
initial_cycle_point = CyclePoint(
description=sstrip('''
'''))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you revert this un-blackening?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Re-blackened the file

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's not what I meant. And it hasn't reverted these changes, which was all I was asking for

@MetRonnie MetRonnie Oct 29, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To make this easier to review, I've reverted the formatting changes that were made to unrelated lines (blackened or un-blackened): acebd40

Comment thread cylc/uiserver/schema.py
else row[f"{prop}_quartile_3"]
),
]
def get_quartiles(row, prop: str) -> List[Optional[float]]:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(Try not to re-introduce the deprecated typing classes going forwards)

list[float | None]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@MetRonnie MetRonnie Oct 29, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not done? But let's leave it as is now

Comment thread cylc/uiserver/schema.py Resolved
Comment thread cylc/uiserver/schema.py
Comment on lines +464 to +473
# fill forwards from the nearest lower quartile
previous = None
for index, value in enumerate(quartiles):
if value is None:
quartiles[index] = previous
else:
previous = value
# fill any leading gaps backwards from the first available quartile
fallback = next((value for value in quartiles if value is not None), None)
return [fallback if value is None else value for value in quartiles]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This can be simpler with a rearrangement:

Suggested change
# fill forwards from the nearest lower quartile
previous = None
for index, value in enumerate(quartiles):
if value is None:
quartiles[index] = previous
else:
previous = value
# fill any leading gaps backwards from the first available quartile
fallback = next((value for value in quartiles if value is not None), None)
return [fallback if value is None else value for value in quartiles]
# find the first existing quartile to use as the initial value
previous = next((value for value in quartiles if value is not None), None)
# fill forwards from the nearest lower quartile
for index, value in enumerate(quartiles):
if value is None:
quartiles[index] = previous
else:
previous = value
return quartiles

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.

4 participants