New IDAaaS data cache index file loading #37187
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
5 tasks
GuiMacielPereira
added
the
ISIS Team: Core
- Created a very simplified implementation and a unit test
Co-authored-by: thomashampson <thomas.hampson@stfc.ac.uk> Co-authored-by: Jonathan Haigh <jonathan.haigh@stfc.ac.uk>
- I made the change because header files from DataHandling could not be imported into files in the API, due to DataHandling being missing from target_link_libraries in the CMakeLists.txt API file. Trying to fix this causes a circular dependency. Co-authored-by: thomashampson <thomas.hampson@stfc.ac.uk> Co-authored-by: Jonathan Haigh <jonathan.haigh@stfc.ac.uk>
Co-authored-by: thomashampson <thomas.hampson@stfc.ac.uk> Co-authored-by: Jonathan Haigh <jonathan.haigh@stfc.ac.uk>
- Added section to look for files in hard coded Linux path - Changed search function to use relative paths - Corrected unit test Co-authored-by: thomashampson <thomas.hampson@stfc.ac.uk> Co-authored-by: Jonathan Haigh <jonathan.haigh@stfc.ac.uk>
- The name of the directories in the cache uses the extended version i.e. 'MARI' instead of 'MAR', changed the function to account for this - Changed split into instr and run to handle edge case of 'SANS2D' (digit in instrument name) Co-authored-by: thomashampson <thomas.hampson@stfc.ac.uk> Co-authored-by: Jonathan Haigh <jonathan.haigh@stfc.ac.uk>
- Function getInstrument() in the ConfigService is used to retrieve the instrument info from a string argument. - The argument of the function is either the name or short name of the instrument - Added option to pass the name or short name + delimiter and retrieve the correct instrument Co-authored-by: Jonathan Haigh <jonathan.haigh@stfc.ac.uk>
- Added unit test for case when there is a delimiter between instrument name and run number - Also checks that short name `PG3` gets properly expanded to `POWGEN` Co-authored-by: Jonathan Haigh <jonathan.haigh@stfc.ac.uk>
- Added some fail proof checks - Added unit tests for these cases Co-authored-by: Jonathan Haigh <jonathan.haigh@stfc.ac.uk>
- Added a test class that creates a dummy directory with the same structure as the data cache and performs unit tests on the path outputs - Introduced safety checks in ISISInstrDataCache to cover for edge cases - Added the data cache path to Mantid properties file Co-authored-by: Jonathan Haigh <jonathan.haigh@stfc.ac.uk>
- When user does not have access permissions to a folder, it will print an error informing the user Co-authored-by: Jonathan Haigh <jonathan.haigh@stfc.ac.uk>
- Deleted unecessary changes that I had made - Replaced instrument finder by one that already accounts for delimiters and other edge cases - Removed unecessary notice() Co-authored-by: Jonathan Haigh <jonathan.haigh@stfc.ac.uk>
GuiMacielPereira
force-pushed
the
37013-new-idaaas-data-cache-index
branch
from
c3291b0
to
1a189a0
Compare
- Added datacachesearch.directory to properties file documentation Co-authored-by: Jonathan Haigh <jonathan.haigh@stfc.ac.uk>
Co-authored-by: Jonathan Haigh <jonathan.haigh@stfc.ac.uk>
- Organized catching of exceptions for bad inputs - Added check for when user does not have permission to open directory - Changed logging from notice level to debug level Co-authored-by: Jonathan Haigh <jonathan.haigh@stfc.ac.uk>
- Made a quick fix for the getInstrument function, to make it throw the exception instead of using a default value - Not proud of this fix, but when I tried to take out the exception handling out of getInstrument, a few other things were breaking Co-authored-by: Jonathan Haigh <jonathan.haigh@stfc.ac.uk>
jhaigh0
requested changes
Apr 24, 2024
There was a problem hiding this comment.
Looking pretty good already, some things to improve.
Other than the comments I've added, I think we can call these files / classes ISISInstrumentDataCache etc. We don't need to cut down on file name length (sorry this is a bit of a pain).
Also, I think getFileParentDirPath could be broken up into smaller functions, I've added one suggestion but there might be other places you could do this.
Co-authored-by: Jonathan Haigh <35813666+jhaigh0@users.noreply.github.com>
Co-authored-by: Jonathan Haigh <jonathan.haigh@stfc.ac.uk>
Co-authored-by: Jonathan Haigh <jonathan.haigh@stfc.ac.uk>
Co-authored-by: Jonathan Haigh <jonathan.haigh@stfc.ac.uk>
Co-authored-by: Jonathan Haigh <jonathan.haigh@stfc.ac.uk>
Co-authored-by: Jonathan Haigh <jonathan.haigh@stfc.ac.uk>
jhaigh0
previously approved these changes
Apr 29, 2024
thomashampson
previously requested changes
May 1, 2024
|
Also be aware there seems to be an MSBuild warning - https://builds.mantidproject.org/job/pull_requests-conda-windows/5886/msbuild/ |
- Check for permission denied is now done inside the loop that directly tries to access files - Added check for non existing parent directory - Improved error message when file is not found whithin parent directory Co-authored-by: Jonathan Haigh <jonathan.haigh@stfc.ac.uk>
- If for some reason the index file on IDAaaS is currupted (eg. being updated) then the error is caught - Allows for the search to continue on the archive Co-authored-by: Jonathan Haigh <jonathan.haigh@stfc.ac.uk>
jhaigh0
requested changes
May 9, 2024
Co-authored-by: Jonathan Haigh <jonathan.haigh@stfc.ac.uk>
jhaigh0
previously approved these changes
May 10, 2024
robertapplin
requested changes
May 10, 2024
sf1919
dismissed
thomashampson’s stale review
The issue with json parsing has been fixed by the DAaaS team.
There was a problem hiding this comment.
Thanks for the changes! @jhaigh0 if you're happy then I'll enable auto merge
robertapplin
approved these changes
May 10, 2024
5 tasks
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
Implemented search in data cache for files before defaulting to search in the data archive. This implementation leaves the previous implementation mostly untouched, and only adds a search in the data cache before advancing to the search in the archive. So the order for searching is User Directories > Data Cache > Data archive. This gives priority to local changes that users have made.
Summary of work
Added
ISISInstrDataCache.handISISInstrDataCache.cppwhich is where the implementation resides. Other than that, changedFileFinder.cppto call the search. Added unit tests.Purpose of work
To give access to data files through the data cache on IDAaaS even when the user does not have access to the archive.
Fixes #37013
Further detail of work
Added a few comments and print statements to make it clear what is going on. We can remove them during the review stage.
To test:
Build to test: https://builds.mantidproject.org/job/build_packages_from_branch/792/
mamba install -c mantid/label/idaas_data_cache_debug mantidworkbench/data/instrument/ALF/ALF_index.json. The numbers on the left side correspond to available run numbersALF_index.jsonfile. You don't need to clickRun, if theOutputWorkspacefield gets updated then the file was successfully found. Try this several times and check they all successfully found the file.99999ALF,1234ALFMAR27340Reviewer
Please comment on the points listed below (full description).
Your comments will be used as part of the gatekeeper process, so please comment clearly on what you have checked during your review. If changes are made to the PR during the review process then your final comment will be the most important for gatekeepers. In this comment you should make it clear why any earlier review is still valid, or confirm that all requested changes have been addressed.
Code Review
Functional Tests
Does everything look good? Mark the review as Approve. A member of
@mantidproject/gatekeeperswill take care of it.Gatekeeper
If you need to request changes to a PR then please add a comment and set the review status to "Request changes". This will stop the PR from showing up in the list for other gatekeepers.