mantidproject / mantid

Main repository for Mantid code
https://www.mantidproject.org
GNU General Public License v3.0
207 stars 122 forks source link

Fix `amend_config` to better orchestrate override of `instrumentDefinition.directory` #37609

Open rboston628 opened 6 days ago

rboston628 commented 6 days ago

Describe the bug

According to Mantid's documentation, all instrument definition files are pointed to by the instrumentDefinition.directory property, and this property can be temporarily overridden inside of a context manager using amend_config. Inside this directory, there should be at least one instrument definition and a facilities.xml file.

All of this documentation suggests that if I have an instrument definition in a local directory, redirecting Mantid to load this IDF by calling it by name should be as easy as

  1. create a facilities.xml file that specifies my instrument, based on the example on the documentation
  2. create an IDF file in the same directory
  3. Use the context manager, like so
    with amend_config(**{"instrumentDefinition.directory": "path/to/my/directory"}):
    LoadEmptyInstrument(InstrumentName="myInstrument", OutputWorkspace="instrument_ws")

However, that doesn't work.

The configuration servicepoints to the directory, but completely ignores the facilities file or any instrument definition files inside the directory, and fails to load any instruments. This can be fixed by adding a statement inside the context manager,

with amend_config(**{"instrumentDefinition.directory": "path/to/my/directory"}):
    ConfigService.updateFacilities("path/to/my/directory/Facilities.xml")
    ConfigService.setFacility("myFacility")

This will still fail, until the following additional steps are taken.

  1. Change the name "facilities.xml" --> "Facilities.xml", with an initial cap. Though the documentation suggests the file should have a lowercase, the updateFacilities() method will only load files that have an initial cap. And moreso, the updateFacilities command more or less entirely ignores the string passed to it and looks for files called "Facilities.xml" within its instrument definition directories, ignoring the name of whatever file is passed to it.
  2. Change the "Facilities.xml" file so that all instruments have a technique added to them. The first example in the documentation is invalid and will fail to parse. Instruments must be listed as
    <instrument name="myInstrument">
    <technique>needs a technique</technique>
    </instrument>

    and not the simpler syntax indicated.

That will enable the following script:

with amend_config(**{"instrumentDefinition.directory": "path/to/my/directory"}):
    ConfigService.updateFacilities("path/to/my/directory/Facilities.xml")
    ConfigService.setFacility("myFacility")
    LoadEmptyInstrument(
        InstrumentName="myInstrument",
        OutputWorkspace = "instrument_ws",
    )

However, when the context manager exits, now it is necessary to undo things further with an additional

ConfigService.updateFacilities("")
ConfigService.reset()

That seems to restore everything to the way ti was before (unverfied).

However, this extra required work is undocumented, contradicts the documentation, and is really only discoverable by several iterations of interpreting error messages.

Additional context

To Reproduce

Create a directory. In that directory, put any valid instrument definition file with the name "ABCDEF_Definition.xml". You can use the SNAPRed test instrument definition, which is valid and never a part of Mantid.

In that directory, also place a copy of the example "facilities.xml" file.

Run this script:

with amend_config(**{"instrumentDefinition.directory": "/path/to/directory"}):
    LoadEmptyInstrument(InstrumentName="ABCDEF", OutputWorkspace="instrument_ws")

That will fail.

Then try the following:

  1. rename the facilities file to "Facilities.xml".
  2. Change the instrument definition in the facilities file to
    <instrument name="ABCDEF">
    <technique> some technique </technique>
    </instrument>
  3. Run this script
    with amend_config(**{"instrumentDefinition.directory": "/path/to/directory"}):
    ConfigService.updateFacilities("")
    ConfigService.setFacility("BrandNew")
    LoadEmptyInstrument(InstrumentName="ABCDEF", OutputWorkspace="instrument_ws")
    ConfigService.updateFacilities("")
    ConfigService.reset()

    That should load the instrument into a workspace called "instrument_ws".

Expected behavior

  1. It should be possible to call updateFacilities() with a file named "facilities.xml", or really named anything at all, so long as the name and file are valid.
  2. The search updateFacilities() performs if the indicated file is not found should be looking for files called "facilities.xml", as indicated in the documentation. It should not require an initial-cap.
  3. Either the parser for "facilities.xml" should allow the simple <instrument name="name"/> syntax, or the documentation should be updated to remove this suggestion.
  4. Using amend_config to override the instrument definition directory should automatically orchestrate with the "facilities.xml" file in this directory to point at those instrument definitions, without requiring more work from the user.
  5. If/when the above change is made, it should also restore all settings changed by the context manager to load the instruments should be reset without requiring more work from the user.

Screenshots

Platform/Version (please complete the following information):

Additional context This came up during development of SNAPRed, which is a consumer of Mantid.

In our unit tests, we often use a small 16-pixel test instrument to test algorithms.

In one unit test, I was testing a loading algorithm we had made for SNAPRed which loads grouping files by calling the correct mantid algorithm based on their extension. In some instances, this requires specifying the instrument definition through either a file, a workspace, or the instrument name.

The tests have a copy of the resulting grouping workspace, then load a grouping file, and compare workspaces. They were doing this on the test instrument because it is fast, and doing this with SNAP or even SNAPLite took a long time.

I needed to test the algorithm loading by name, but the test instrument wasn't actually registered in Mantid. Overriding the instrument directory is the correct way to get Mantid to point to my instrument definition file, but caused this extra headache.