-
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
Suite srv files mgr #3334
Suite srv files mgr #3334
Conversation
This will fail unit-tests. Mock-based unit-tests are inherently fragile to implementation changes (which is annoying as that is when you need them the most). Two are still failing, I can't work out how to fix them without re-implementing them.
I can't seem be able to patch imported functions the way it was done before e.g: mocked_os.path.basename = os.path.basename @kinow any advice on how to fix these tests? |
Created a PR in case that's easier to copy/adapt the code from: oliver-sanders#10 |
c18593c
to
41f18e7
Compare
Aah, OK, I've no idea why this change would break that.
Thanks, would have taken me forever to work that out. |
Looks like there might be a small speed-up, I've measured an improvement of ~10% in my slow, bloated Conda environment. $ git checkout master
$ cylc run generic --hold
$ prof --n=10 --shell=python 'from cylc.flow.network.client import SuiteRuntimeClient; SuiteRuntimeClient("generic")'
1.91900000000000000000
$ git checkout suite-srv-files-mgr
$ prof --n=10 --shell=python 'from cylc.flow.network.client import SuiteRuntimeClient; SuiteRuntimeClient("generic")'
1.69400000000000000000 |
Deconflicted. |
Merged in latest master with TB tweaks. |
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.
Quick comment.
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.
Approving as @oliver-sanders has addressed some comments and argued his way out of the others 👍
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.
Added some comments, but I think these are no blockers @oliver-sanders . Feel free to press the merge button if OK (I guess we can more renaming class name and adding docstrings to another follow-up PR or issue?)
And code looking much cleaner 👏 👏 👏 |
7cde314
to
958982f
Compare
Addressed comments, waiting for tests to pass... |
958982f
to
89c1a80
Compare
Deconflicted. |
(coverage is actually up slightly but the patch coverage is low because suite_srv_files_mgr coverage was low before) |
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.
One unused import, but not a blocker. Re-approved 👍
This is a
smallinternal change with no associated Issue.This came out of the import time investigations we were looking into the other day.
Something which has long bugged me it that we have to initialise the
SuiteSrvFilesManager
class every time we want to use it despite the fact that most of it's methods could be functions.This PR:
suite_srv_files_mgr
module purely functional.Note I didn't use Python
enum.Enum
for the constants but just dumped them in classes (for namespaceing). The reason for this is that enums are a bit of a faff (you end up adding lots ofMyEnum.Foo.value
lines).Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.