-
Notifications
You must be signed in to change notification settings - Fork 93
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
tokens: speed up duplication of Tokens objects #5325
Conversation
3803e91
to
2219798
Compare
Co-authored-by: Ronnie Dutta <61982285 [email protected]>
Nice! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the integration tests, the id
is currently a posix path which is breaking them with this change. I'm guessing mypy doesn't check tests.
This diff fixes it:
diff --git a/tests/integration/test_reinstall.py b/tests/integration/test_reinstall.py
index 02be406bc..8b13c5574 100644
--- a/tests/integration/test_reinstall.py
b/tests/integration/test_reinstall.py
@@ -80,7 80,7 @@ def one_run(one_src, test_dir, run_dir):
)
return SimpleNamespace(
path=w_run_dir,
- id=w_run_dir.relative_to(run_dir),
id=str(w_run_dir.relative_to(run_dir)),
)
That's a genuine test fail, in if args:
if len(args) > 1:
raise ValueError()
if isinstance(args[0], str):
kwargs = tokenise(args[0], relative)
else:
> kwargs = dict(args[0])
E TypeError: 'PosixPath' object is not iterable
cylc/flow/id.py:113: TypeError |
Fix? diff --git a/tests/integration/test_reinstall.py b/tests/integration/test_reinstall.py
index 02be406bc..8b13c5574 100644
--- a/tests/integration/test_reinstall.py
b/tests/integration/test_reinstall.py
@@ -80,7 80,7 @@ def one_run(one_src, test_dir, run_dir):
)
return SimpleNamespace(
path=w_run_dir,
- id=w_run_dir.relative_to(run_dir),
id=str(w_run_dir.relative_to(run_dir)),
) |
Argghhh, @datamel bet me to it!!! (she must have been working late!). Still, same fix suggested - great minds think alike 😁 |
Ta for the fix both :) |
Added a collective changelog for all three efficiency changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(only linkcheck failing)
Sometimes we need to duplicate a
Tokens
object.Currently this works by doing
tokenise(detokenise(tokens))
which is really inefficient (I should know better, I've moaned about this in the cycler classes before!).This PR switches to using the already parsed values held in the
tokens
object.Using the example from #5315 with
-s TASKS=25 -s CYCLES=1
this reduces the time taken byincrement_graph_window
by ~50% after the changes from #5319 and #5321 have been applied.Before (with #5319 & #5321)
After (still with #5319 & #5321)
Check List
CONTRIBUTING.md
and added my name as a Code Contributor.setup.cfg
andconda-environment.yml
.CHANGES.md
entry included if this is a change that can affect users?.?.x
branch.