Check Data Directories on Request, Not Startup #28760
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cailafinn
added
Framework
cailafinn
force-pushed
the
startup-directory-checks
branch
from
2a72275
to
9107d05
Compare
cailafinn
force-pushed
the
startup-directory-checks
branch
8 times, most recently
from
ab55571
to
3da6c13
Compare
martyngigg
previously approved these changes
Jun 4, 2020
There was a problem hiding this comment.
Confirmed using wondershaper to slow connection to 10kb/s down and up. Adding a relative path to a network drive to the datasearch directories caused
from mantid.kernel import ConfigService
ConfigService.Instance()
to slow down from ~0.4s to ~1.5s.
With these changes the startup time of ConfigService is now independent of the directories present.
|
@ConorMFinn Lots of build failures here, can you take a look |
|
@ConorMFinn Just one more failing test on Windows now. |
cailafinn
force-pushed
the
startup-directory-checks
branch
2 times, most recently
from
58353f0
to
1dcde7f
Compare
martyngigg
reviewed
Jun 10, 2020
cailafinn
force-pushed
the
startup-directory-checks
branch
from
1dcde7f
to
eeabcd0
Compare
It's a legacy key that is no longer in active use.
If the key contains the string "director" it is assumed to point to a directory and if it its value is relatiove then the path is converted to an absolute path. This reduces the workload on startup while keeping the caching of important keys such as the datasearch paths and instrument directories for performance. Refs #12425
Config service now converts to an absolute path when storing values. Tests needed some changes to account for this. RE #28499
A lot of the ILL tests were using the appendDataSearchSubDir method in the setup functions. This was resulting in many directories being added to the config service as the tests ran. They have now been moved to the setUpClass functions or constructors so they are only added once. RE #28499
The changes to the ConfigService means that it can hold invalid paths until they are requested. This means that a check needed adding to ensure a path was valid before doing anything with it. RE #28499
cailafinn
force-pushed
the
startup-directory-checks
branch
from
eeabcd0
to
355dbe1
Compare
martyngigg
approved these changes
Jun 10, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of work.
Changes the behaviour of the ConfigService so that data directories are checked when access is reqested, rather than on startup. The intention is to reduce the startup time of workbench when checking for network drives that may be very slow to access.
To test:
tricklemight be useful)Refs #28499
Reviewer
Please comment on the following (full description):
Code Review
Functional Tests
Does everything look good? Mark the review as Approve. A member of
@mantidproject/gatekeeperswill take care of it.