38882 gsas2scriptable path search revision #38976
Conversation
JackEAllen
added
Diffraction
Create custom implementation of rglob with the method name limited_rglob to recursively search a directory tree structure for a given filename. limited_rglob allows for the setting of a max depth value to limit how deep the directory tree structure is searched to avoid searching uneccesarily deep which could be time-consuming and recourse intensive depening on the path provided
Replace previous approach which directly set the import path and attempted to import the module with a more robust and dynamic approach. Replacing the previous implementation with find_gsasii allows for controlled depth resursive searching of sub-directories for GSASIIscriptable and does not rely on hard-coded paths which may change across versions of GSAS-II. Moving the previous approach into find_gsasii improves error handling and error visibility by providing more detailed errors which will help diagnose if GSASIIscriptable has been found and there is an import error or if GSASIIscriptable could not be found.
Improve error description when searching for GSASII binaries if they cannot be found. It is not inherently obvious that the outer most parent path of the GSASII installation is required in the settings panel for the model as only GSASIIscriptable is required to import GSAS in call_G2sc.py. The improved error message tells the user where they should set the path too rather than assuming
JackEAllen
force-pushed
the
38882_gsas2scriptable_path_search_revision
branch
from
04e9742 to
a251cce
Compare
There was a problem hiding this comment.
This is a big improvement thanks for doing this! I have tested on windows with a newer version and everything works great and the error messages are a lot more helpful.
I have yet to test on IDAaaS as they seem to have network issues, in the meantime I have a minor suggestion for the release note. Also what do you think about adding a unit test?
| @@ -0,0 +1 @@ | |||
| - `#38882 <https://github.com/mantidproject/mantid/issues/38882>`_ : Fix issue with ``GSAS-II GSASIIscriptable.py`` hard-coded path which is invalid for newer version of GSAS-II. | |||
There was a problem hiding this comment.
Probably worth adding a version number which we know doesn't work on main? Just so people get a feel for what newer versions of GSAS-II means?
There was a problem hiding this comment.
Good point! There may be other older versions of GSAS-II between the one installed on IDAaaS (4579) and the most recent version I have tested with (5758) that may also cause a path issue prior to this PR that I'm not aware of, however we should specify at least one known version that breaks on main currently. I will update the release notes to include the newest version of GSAS-II 5758 that this PR aims to resolve.
There was a problem hiding this comment.
Yep I think there's no point going through GSAS github trying to find out when the directory changes, but agreed, if you could add that it will now work for version 5758 as you suggest - that would be great thanks!
| @@ -164,6 +168,41 @@ def export_lattice_parameters(temp_save_directory, name_of_project, project): | |||
| file.write(parameters_json) | |||
|
|
|||
|
|
|||
| def limited_rglob(directory: Path, pattern: str, max_depth: int) -> Generator[Path, None, None]: | |||
There was a problem hiding this comment.
Technically this could have a unit test associate with it in test_call_G2sc.py - perhaps setting up a temp directory that reflects the old and new GSAS version setup? But that might be overkill - what do you think?
Don't worry about making any changes as this seems to work well. However, it occurred to me the logic in this function is quite complicated for what it is doing (but maybe I'm not grasping all the subtleties)!
For example on L177
if depth >= max_depth:
dirs[:] = [] # Max depth reached, don't recurse further
Is there any reason one couldn't break here and return the appropriate null path (anything that isn't a file I think)?
Could depth instead just be a counter and do depth += 1 instead of depth = root[len(str(directory)) + len(os.path.sep) :].count(os.path.sep) + 1 etc.?
There was a problem hiding this comment.
Using break would exit the loop entirely, which might mean we don't find the file we are searching for if there are other directories at the same level to process.
By using dirs[:] = [], we ensure that the traversal stops at the current directory level without affecting the parent directory's traversal if there are other directories at at the same level. This approach basically future proofs against the path ever changes again.
Using depth = root[len(str(directory)) + len(os.path.sep) :].count(os.path.sep) + 1 will have the downside of being more complicated compared to using a counter due to the usage of string slicing and counting separators, but will be more accurate.
The depth is recalculated based on the actual path, ensuring it accurately reflects the directory structure which will be resilient against the directory structure possibly changing during execution (though this is very unlikely). The main benefit is that this will also handle edge cases where directories might be skipped or have varying depths.
I agree that this method is likely too complicated for just the case of searching for GSASIIscriptable.py.
My plan is to potentially move this method out of engineering_diffraction/tabs/gsas2/call_G2sc.py and into engineering_diffraction/tabs/gsas2/model.py as part of a larger refactor and use the same method to also search for the python binaries which are searched for within mantid/qt/python/mantidqtinterfaces/mantidqtinterfaces/Engineering/gui/engineering_diffraction/tabs/gsas2/model.py.call_subprocess to protect against possible being moved elsewhere within the GSAS-II package.
There was a problem hiding this comment.
Though adding a test could be considered overkill as path searching errors will be caught elsewhere when searching for GSASIIscriptable.py I believe you're right to suggest adding a test. Thank you for catching this!
I'll work on adding a test to check a variety of different directory structures as this will also be a useful test to have for when this method is possibly moved into mantid/qt/python/mantidqtinterfaces/mantidqtinterfaces/Engineering/gui/engineering_diffraction/tabs/gsas2/model.py
There was a problem hiding this comment.
Thanks for adding a test!
RE logic/counters etc. - I didn't mean drag and drop the counter etc. into the current logic. I know it would require different ifs and loops etc. To be clear I'm not suggesting you change what you have! Also I hadn't thought of the directory changing mid-execution - a good point!
There was a problem hiding this comment.
I'll refresh the review once I've added a test 👍
Description of work
Summary of work
Dynamically search for
GSASIIscriptable.pyfrom outer most parent library of GSAS-II using a recursive directory search with a depth limit of 3 set to avoid searching too deeply into a given directory structure.Improve error message return descriptions where:
GSASIIscriptable.pyis not found.GSASIIscriptable.pyhas been found, but an import error occurred./gsas2_parent_dir/GSAS-II/bin/pythonThe location of
GSASIIscriptable.pyis not consistent across different versions of GSAS-II. For newer version of GSAS-II (5758 or later),GSASIIscriptable.pyis not correctly locatedFixes #38882
Further detail of work
Mantid anaconda label:
new_gsas_path_search_test- PLEASE REMOVE LABEL FROM ANACONDA ONCE PR IS APPROVED.To test:
Testing with newer versions of GSAS-II:
mainbranch:git switch main && git fetch && git pullworkbenchand follow the Engineering Diffraction Test 11GSASIIscriptable.py, maybegsas2/GSAS-II, you will receive an similar error to:git switch 38882_gsas2scriptable_path_search_revision3-5, and notice that the error for step4GSAS should now load correctly. For step5, notice that the same error will be returned with the additional statement:Path must be outer most directory of GSAS-II installation.This message should better aid a user to the directory needed for GSAS-II to load correctly.GSASIIscriptable.pyto another directory within the GSAS library such as directly under the parent folder. the rough location ofGSASIIscriptable.pyfrom the parent directory of the library will be:/gsas2/GSAS-II/GSASII/GSASIIscriptable.pyfor newer versions of GSAS-II3. You should receive an import error that specified thatGSASIIscriptable.pyhas been found, but an import error occurred similar to:GSASIIscriptable.pyback to it's original location: ~/gsas2/GSAS-II/GSASII/GSASIIscriptable.pyTesting with older versions of GSAS-II:
4579. This can be checked before launching the workspace by clicking on the setting cog at the top right of the workspace under the "Included Software" subheading.Applications >> System >> Terminal-c mantid/label/nightlyis required due to a an mslice version being pinned which will prevent you from creating a valid environment otherwise.6. Activate your environment
mamba activate mantid-testand open workbenchworkbenchfrom the CLI.7. Repeat steps
3-5, from "Testing with newer versions of GSAS-II" and notice the update error messages, mentioned in step7.Reviewer
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.