replace pkg_resources with importlib_metadata and packaging #5790

oliver-sanders · 2023-10-25T11:05:12Z

Closes Replace pkg_resources #5775
Replace the pkg_resources library (deprecated) with newer alternatives.
This resolves an efficiency issue due to the high import time of pkg_resources. This import time hits every single Cylc command including cylc message.

Test Import Time

The pkg_resources library can take a glacial age to import. To see this for yourself and test the difference try this:

# test.py

# the imports required to call the cylc message script
from cylc.flow.scripts import cylc
from cylc.flow.scripts import message

$ python -X importtime test.py 2> import.log
$ pip install tuna
$ tuna import.log

Results:

Before	After

Test CLI Time

To measure the impact of this change on end-to-end performance, I used this diff to turn off the actual work of cylc message:

diff --git a/cylc/flow/task_message.py b/cylc/flow/task_message.py
index 3ea753e73..e4b96f5bf 100644
--- a/cylc/flow/task_message.py
+++ b/cylc/flow/task_message.py
@@ -81,6 +81,7 @@ def record_messages(workflow: str, job_id: str, messages: List[list]) -> None:
         job_id: Job identifier "CYCLE/TASK_NAME/SUBMIT_NUM".
         messages: List of messages "[[severity, message], ...]".
     """
+    return
     # Record the event time, in case the message is delayed in some way.
     event_time = get_current_time_string(
         override_use_utc=(os.getenv('CYLC_UTC') == 'True'))

Note, without this diff, execution times vary wildly due to IO.

Results (average of 5 calls):

Before: 0.832s
After: 0.516s

So a ~0.3 second difference, which is good, but less of a saving than the ~1s indicated by python -X importtime.

I don't understand the disparity between the two, but at least the number is heading in the right direction.

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).
CHANGES.md 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.

cylc/flow/scripts/cylc.py

cylc/flow/__init__.py

setup.cfg

hjoliver · 2023-10-29T19:51:51Z

This resolves an efficiency issue due to the high import time of pkg_resources. This import time hits every single Cylc command including cylc message.

Great. Cylc 8 command start-up time on our HPC can be pretty dire.

oliver-sanders · 2023-10-30T10:21:27Z

Great. Cylc 8 command start-up time on our HPC can be pretty dire.

This combined with #5770 could see a saving of up to 2s on cylc message calls which were taking way too long.

They now take ~1.5s for me (results and savings may differ wildly based on FS characteristics):

$ /usr/bin/time -v cylc message generic 1234 foo whatever 2>&1 | grep Elapsed
	Elapsed (wall clock) time (h:mm:ss or m:ss): 0:01.53
$ /usr/bin/time -v cylc message generic 1234 foo whatever 2>&1 | grep Elapsed
	Elapsed (wall clock) time (h:mm:ss or m:ss): 0:01.69
$ /usr/bin/time -v cylc message generic 1234 foo whatever 2>&1 | grep Elapsed
	Elapsed (wall clock) time (h:mm:ss or m:ss): 0:01.19

Of which ~0.6 seconds is the actual work of sending / writing messages. The remainder being overheads including the time taken to import the required code. So it should be possible to bring this number down further.

I think the main low-hanging fruit is in splitting up overgrown modules to prevent CLI commands from importing more than they need to (e.g. #5369).

MetRonnie · 2023-10-30T10:34:52Z

$ cylc help all
Traceback (most recent call last):
  File "~/.conda/envs/cylc8/bin/cylc", line 8, in <module>
    sys.exit(main())
  File "~/github/cylc-flow-8/cylc/flow/scripts/cylc.py", line 663, in main
    print_command_list()
  File "~/github/cylc-flow-8/cylc/flow/scripts/cylc.py", line 453, in print_command_list
    contents = [
  File "~/github/cylc-flow-8/cylc/flow/scripts/cylc.py", line 453, in <listcomp>
    contents = [
  File "~/github/cylc-flow-8/cylc/flow/scripts/cylc.py", line 420, in iter_commands
    module = __import__(entry_point.module_name, fromlist=[''])
AttributeError: 'EntryPoint' object has no attribute 'module_name'

oliver-sanders · 2023-10-30T10:42:50Z

(fixed and added test)

MetRonnie · 2023-10-30T13:51:19Z

IMO we should move the (GH Actions) linkcheck step into the lint workflow too

MetRonnie

👍

.github/workflows/test_fast.yml

oliver-sanders · 2023-10-30T15:47:36Z

(may need to fiddle the lint timeout, let's see what it does)

.github/workflows/test_fast.yml

MetRonnie · 2023-10-30T21:19:59Z

.github/workflows/test_fast.yml

        run: |
-          bandit -r --ini .bandit cylc/flow
+          pytest cylc/flow tests/unit


If you're consolidating these into one step, it would be good to set -q for quiet in PYTEST_ADDOPTS, to prevent us having to scroll thousands of log lines to get to the test failures. Note that when CI=true Pytest always outputs full diffs in assertion failures regardless of the verbosity option.

Also -l for displaying the value of locals in traceback might be handy.

I don't see combining these sections as a reason for reducing verbosity, better to have the required information than to hide it?

(It didn't really make sense to separate doc and unit tests they are all unit tests and splitting them just slows things down)

I'm talking about hiding these (4000!) lines without reducing the verbosity of failed assertion output

I don't really see a problem with this and seeing the test output is helpful for debugging timeouts. Unrelated either way.

MetRonnie · 2023-10-31T10:58:51Z

_remote_background_indep_tcp failure looks genuine

====== /home/runner/cylc-run/_remote_background_indep_tcp/cylctb-20231031T104705Z-KzdI/f/broadcast/09-remote/log/job/19990101T0000Z/t1/01/job.err ======
Sending DEBUG MODE xtrace to job.xtrace
"cylc broadcast" requires "cylc-flow"

ModuleNotFoundError: No module named 'packaging'
Traceback (most recent call last):
  File "/cylc/cylc/flow/task_message.py", line 108, in send_messages
    pclient = get_client(workflow)
              ^^^^^^^^^^^^^^^^^^^^
  File "/cylc/cylc/flow/network/client_factory.py", line 63, in get_client
    return get_runtime_client(get_comms_method(), workflow, timeout=timeout)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/cylc/cylc/flow/network/client_factory.py", line 54, in get_runtime_client
    from cylc.flow.network.client import (  # type: ignore[assignment]
  File "/cylc/cylc/flow/network/client.py", line 45, in <module>
    from cylc.flow.network.server import PB_METHOD_MAP
  File "/cylc/cylc/flow/network/server.py", line 31, in <module>
    from cylc.flow.network.graphql import (
  File "/cylc/cylc/flow/network/graphql.py", line 42, in <module>
    from cylc.flow.network.schema import NODE_MAP, get_type_str
  File "/cylc/cylc/flow/network/schema.py", line 41, in <module>
    from cylc.flow.broadcast_mgr import ALL_CYCLE_POINTS_STRS, addict
  File "/cylc/cylc/flow/broadcast_mgr.py", line 30, in <module>
    from cylc.flow.cfgspec.workflow import SPEC
  File "/cylc/cylc/flow/cfgspec/workflow.py", line 26, in <module>
    from cylc.flow.cfgspec.globalcfg import (
  File "/cylc/cylc/flow/cfgspec/globalcfg.py", line 25, in <module>
    from packaging.version import parse as parse_version, Version
ModuleNotFoundError: No module named 'packaging'
2023-10-31T10:47:15Z CRITICAL - failed/ERR

* Closes cylc#5775 * Replace the pkg_resources library (deprecated) with newer alternatives. * This resolves an efficiency issue due to the high import time of pkg_resources. This import time hits every single Cylc command including `cylc message`.

oliver-sanders · 2023-10-31T11:41:24Z

I had missed packaging off of the conda file, added it now, should pass.

oliver-sanders added this to the cylc-8.3.0 milestone Oct 25, 2023

oliver-sanders self-assigned this Oct 25, 2023

oliver-sanders marked this pull request as ready for review October 25, 2023 13:43

oliver-sanders added the efficiency For notable efficiency improvements label Oct 26, 2023

oliver-sanders force-pushed the 5775 branch 3 times, most recently from c74f642 to 575fb13 Compare October 26, 2023 11:57

MetRonnie reviewed Oct 27, 2023

View reviewed changes

cylc/flow/scripts/cylc.py Outdated Show resolved Hide resolved

cylc/flow/__init__.py Outdated Show resolved Hide resolved

setup.cfg Show resolved Hide resolved

oliver-sanders force-pushed the 5775 branch from 575fb13 to 9b56e5d Compare October 30, 2023 10:08

oliver-sanders force-pushed the 5775 branch from 9b56e5d to 0490fd7 Compare October 30, 2023 10:42

oliver-sanders force-pushed the 5775 branch from 0490fd7 to 8151e70 Compare October 30, 2023 13:35

MetRonnie approved these changes Oct 30, 2023

View reviewed changes

oliver-sanders force-pushed the 5775 branch from 8151e70 to a0897bf Compare October 30, 2023 15:32

MetRonnie reviewed Oct 30, 2023

View reviewed changes

.github/workflows/test_fast.yml Outdated Show resolved Hide resolved

MetRonnie approved these changes Oct 30, 2023

View reviewed changes

MetRonnie reviewed Oct 30, 2023

View reviewed changes

.github/workflows/test_fast.yml Show resolved Hide resolved

oliver-sanders force-pushed the 5775 branch from e3bb1ee to b8ff0f2 Compare October 30, 2023 17:43

MetRonnie reviewed Oct 30, 2023

View reviewed changes

oliver-sanders added 2 commits October 31, 2023 11:40

actions: move lint checks into their own job

a351f09

oliver-sanders force-pushed the 5775 branch from b8ff0f2 to a351f09 Compare October 31, 2023 11:40

MetRonnie approved these changes Oct 31, 2023

View reviewed changes

MetRonnie added the dependencies label Oct 31, 2023

oliver-sanders requested review from hjoliver and wxtim and removed request for hjoliver November 1, 2023 10:19

wxtim approved these changes Nov 8, 2023

View reviewed changes

wxtim merged commit b4e1117 into cylc:master Nov 8, 2023
35 of 38 checks passed

oliver-sanders deleted the 5775 branch November 8, 2023 10:22

oliver-sanders mentioned this pull request Apr 23, 2024

pkg_resources: move to importlib.metadata metomi/rose#2758

Closed

oliver-sanders mentioned this pull request Oct 18, 2024

cylc commands from a directory that does not exist should not cause failures (generally) #5688

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace pkg_resources with importlib_metadata and packaging #5790

replace pkg_resources with importlib_metadata and packaging #5790

oliver-sanders commented Oct 25, 2023 •

edited

Loading

hjoliver commented Oct 29, 2023

oliver-sanders commented Oct 30, 2023 •

edited

Loading

MetRonnie commented Oct 30, 2023 •

edited

Loading

oliver-sanders commented Oct 30, 2023

MetRonnie commented Oct 30, 2023 •

edited

Loading

MetRonnie left a comment

oliver-sanders commented Oct 30, 2023 •

edited

Loading

MetRonnie Oct 30, 2023 •

edited

Loading

oliver-sanders Oct 31, 2023 •

edited

Loading

MetRonnie Oct 31, 2023 •

edited

Loading

oliver-sanders Oct 31, 2023 •

edited

Loading

MetRonnie commented Oct 31, 2023 •

edited

Loading

oliver-sanders commented Oct 31, 2023

replace pkg_resources with importlib_metadata and packaging #5790

replace pkg_resources with importlib_metadata and packaging #5790

Conversation

oliver-sanders commented Oct 25, 2023 • edited Loading

Test Import Time

Test CLI Time

hjoliver commented Oct 29, 2023

oliver-sanders commented Oct 30, 2023 • edited Loading

MetRonnie commented Oct 30, 2023 • edited Loading

oliver-sanders commented Oct 30, 2023

MetRonnie commented Oct 30, 2023 • edited Loading

MetRonnie left a comment

Choose a reason for hiding this comment

oliver-sanders commented Oct 30, 2023 • edited Loading

MetRonnie Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

oliver-sanders Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

MetRonnie Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

oliver-sanders Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

MetRonnie commented Oct 31, 2023 • edited Loading

oliver-sanders commented Oct 31, 2023

oliver-sanders commented Oct 25, 2023 •

edited

Loading

oliver-sanders commented Oct 30, 2023 •

edited

Loading

MetRonnie commented Oct 30, 2023 •

edited

Loading

MetRonnie commented Oct 30, 2023 •

edited

Loading

oliver-sanders commented Oct 30, 2023 •

edited

Loading

MetRonnie Oct 30, 2023 •

edited

Loading

oliver-sanders Oct 31, 2023 •

edited

Loading

MetRonnie Oct 31, 2023 •

edited

Loading

oliver-sanders Oct 31, 2023 •

edited

Loading

MetRonnie commented Oct 31, 2023 •

edited

Loading