#380 External scan recorders

[Jan Kotanski]

Hello Sardana Developers,

after one year I've decided to refresh discussions about external
scan recorders. I attach my proposal.

I've added a ScanRecorderPath property to tango MacroServer.
By default they are read from macroserver/scan/recorder.

Probably, if we would like to support external recorders it would be
good to change DataFormats. Now, it is enum so it is by definition not
extendable.

Cheers,
Jan

On 09/08/15 14:20, Zbigniew Reszela wrote:

Dear Jan,

First of all sorry that you have to wait so long, but finally we reviewed your
patch. Below our comments:

Some naming comments :

• msscanrecordermanager module rename to msrecordermanager

• ScanRecorderManager class rename to RecorderManager

Further implementation:

• In sardana.macroserver.msrecordermanager.RecorderManager:
  -- maintain a list of recorders, not just the modules. For the moment I would
  just discover classes inheriting from
  sardana.macroserver.scan.recorder.BaseFileRecorder class.
  -- add getter of a Recorder class (getRecorderClass(name))
  -- add getter of all Recorder classes (getRecorderClasses())

• In sardana.macroserver.MacroServer:
  -- add get_recorder_class which should look and return the Recorder class from
  the RecorderManager
  -- add get_recorder_classes which should look and return the Recorder class from
  the RecorderManager

• Customization of the Recorders could be achieved by extending syntax of the
  ScanFile environment variable:
  -- NOW -> either string with name or list of them
  -- FUTURE -> either string with name or tuple with name and format or list of
  them allowing combination of both e.g. list(“file1.dat”, tupe(“file2.txt”,
  “MyRecorder”))

• Modify the sardana.macroserver.scan.gscan.GSscan class to interpret the new
  syntax of ScanFile and pass the correct arguments to FileRecorder factory.

• Change sardana.macroserver.scan.recorder.storage.FileRecorder factory:
  -- so it accepts optional FileRecorder class name
  -- in case the FileRecorder class is not passed, look for the FileRecorder hint
  in the macro - this usages should be marked as deprecated
  -- if neither FileRecorder was passed nor the macro defines the hint, it should
  look for matching between the file extension and the Recorder formats. and use
  the first Recorder implementing the necessary format
  -- finally it should fallback to SPEC recorder

All these are open for discussion.
If you agree in going in this direction I propose to open a remote branch and
work on it together.

What do you think?
Zibi on behalf of our development team:)

Hi Jan,

I write my comments below.

On 09/08/2015 03:38 PM, Jan Kotanski wrote:

Hi Zibi,

Great. Yes, we can open the remote branch (any proposition?) and work on
it together.
We have already a fork, where we work on SEP6 for example, we could create a
branch there and give you permissions to commit.
Let's create a feature request in the SF tickets as well, so we could refer to
the branch and maybe move the discussion there? What do you think?

For me the new ScanFile structure seems to be too complicated for users,
e.g. in spock

senv ScanFile [(\"scan1234.fio\",\"FIO_FileRecorder\"),
[(\"scan1234.nxs\",\"NXS_FileRecorder\")]

but I don't have any other good solution for it.

Yes, regarding the variable there are some pros and cons of going into more
complex syntax.
Another alternative could be to introduce a new variable, with just Recorders.
I think that users do not change recorders as often as file names, so you are
right that introducing the new syntax would make their life more difficult.
So another proposal is:
Keep ScanFile variable as it is, and introduce ScanRecorder variable allowing
string or list of strings with names of the recorders.
It should be consistent with the ScanFile variable in the sense of length and of
order of names.

Yes, in my version I decided for managing the modules to make only
minimal changes in MacroServer. But the proposed FUTURE version with
getRecorder* methods is of course also OK.

I think it is more complete and would allow us to get rid of some "technical
debt" code in the FileRecorder factory...

Cheers,
Jan

Many thanks!
Zibi

[Carlos Falcon]

Dear all,
We have created a branch feature-380 for the colaborating development in our fork.
You can follow the developing of this feature, here git://git.code.sf.net/u/cmft/sardana-GenericSoftware.

For participate in the development or test the code, follow the next git commands:

git remote add NAME_OF_YOUR_REMOTE git://git.code.sf.net/u/cmft/sardana-GenericSoftware
git remote update
git branch -b feature380 NAME_OF_YOUR_REMOTE/ feature-380

Although all of you can participated sending patches, If any of you would like to collaborate with the development don't hesitate to ask me for permissions.

PS: Jan I gave you permissions. Welcome to the "international" ctgensoft!!.

[Jan Kotanski]

Thanks.

BTW, the last command should be

git checkout -b feature380 NAME_OF_YOUR_REMOTE/ feature-380

[Jan Kotanski]

Hi,

Accidentally I push my develop and master branch into cmft.
Do you know How to revert it easily.

Best regards,
Jan

[Jan Kotanski]

Hi,

As a starting point I've pushed my patch into feature-380. It can be tested with the wheezy package

python-sardana-scanrecorders

from

deb http://repos/pni-hdri.de/apt/debian wheezy main

The above package contains NeXus recorder used by DESY.

Cheers,
Jan

[Jan Kotanski]

The correct http adress from previous is:

deb http://repos.pni-hdri.de/apt/debian wheezy main

[Carlos Falcon]

Hi all,
These are some design decision taken via private emails.
We think that the best implementation is to allow the definition of an optional environment variable,
"FileRecorder" that is a string or a list of string of file recorder name(s).

Then, add to FileRecorder factory in SardanaCO/src/sardana/macroserver/scan/recorder/storage a new argument, that will be filled by gscan API.

So, the gscan API has to be changed and it is the responsible to manage the correlation between SpecFiles and ScanRecorders.

def FileRecorder(filename, macro, scanrecorder=None, **pars):

And the logic of this method will be:

1- If scanrecorder is not None: Ask to the recordermanager for get the scanrecorder_klass of the given scanrecorder name.
2- If not, See if there is a macro HINT (This is deprecated )
3- If not, Modify the current implementation base on extensions. Using a sardanacustomsettings variable. lets say "ScanRecorderMap", that is a dictionary with the relationship between Recoders and file extensions.
by the default (even if it is no defined) it should has to preserve the current codification. If you are not familiar with the sardanacustomsettings, we can help you with this implementation.

Just an example (this must be the default definition of SCAN_RECORDER_MAP for back-comp):

1 2 3 4 5 6 7

#!/usr/bin/python SCAN_RECORDER_MAP = { '.h5': 'NXscan_FileRecorder', '.h4': 'NXscan_FileRecorder', '.xml': 'NXscan_FileRecorder', '.spec': 'SPEC_FileRecorder', '.fio': 'FIO_FileRecorder'}

This way allows the user uses different default recorder per extensions.

1 2 3 4 5 6 7 8

#!/usr/bin/python SCAN_RECORDER_MAP = { '.h5': 'MyNXscan_FileRecorder', '.h4': 'NXscan_FileRecorder', '.xml': 'NXscan_FileRecorder', '.spec': 'SPEC_FileRecorder', '.fio': 'FIO_FileRecorder', '.dat': 'FOO_FileRecorder'}

4- Use the SPEC_FileRecorder as default.

Note: For the moment we don't think that the intermediate recorder class can cause us problems, so the recorder manager has to discover all of them. So, all recorders will be presented as valid recorders (Giving the responsibility of choose the right recorder to the users)

Best,
Carlos Falcon

[Jan Kotanski]

Hi,

The problem is that SCAN_RECORDER_MAP is defined in a python file in PYTHONPATH so after installation neither user is allowed to changed it nor other program e.g. other debian package.

That means that without defined ScanRecorder only built-in recorders will be available.
This causes we have two classes of recorders: better -- built-in, and worse -- external one. Therefore the bad thing in the proposed implementation is:

• user has to learn by heart which recorder is build in and which one is external. Since the current status will be changed this will be misleading for users.

• DESY NeXus recorder belongs to external one so after installing a new version of sardana it will disappear for the users (without defining ScanRecorder). There DESY cannot accept it without any modifications. In DESY this would lead to further use of the current procedure with our separate Sardana branch (without External Recorders).

I see the following options:

1. remove support for all file extension, i.e remove SCAN_RECORDER_MAP. That means using ScanRecorder variable is mandatory otherwise data is written in SPEC file.
   (Personally I don't like this solution because if user does not specify ScanRecorder she/he can get SPEC file inside e.g. 'data.nxs'. Actually this is also a feature the proposed by Carlos implementation)

2. add automatically all external recorders to SCAN_RECORDER_MAP. That is a minimal change of the proposed in the previous post implementation which support in equal way build-in and external recorders. (Actually it is implemented in feature380, commit 0850ac8dd5e7ddf62ee4b2dfe5876b2e2f6f06f4)

3. use only file extension filter without adding ScanRecorder variable. That may cause problems when we have more Recorders which correspond to one extension, like current NeXus implementation from ALBA. The workaround for this is to put those recorders into separate packages and use RecorderPath property to specify the only required recorder.

4. two step selection: file extensions and the ScanRecorder variable. That would mean that recorder is selected according to its extension, and if it is not possible to choose it uniquely then the choice is specified by the ScanRecoder variable. (Than would protect against a usage of non-matching extension to recorders).

Since after internal discussion (me and ALBA developers) we couldn't choose the unique solution we've decided to ask of all sardana developers and users for their opinion or other propositions. The final decision could be taken on Sardana Workshop.

In my opinion users stick to use file extensions to select the proper file format so my tips are:

(4.) and (2.): [the best]
(3.): [OK]
(1.): [acceptable]
(0.): [from the Carlos' post - unacceptable]

Bests,
Jan

[Jan Kotanski]

Hello Fred,

Thanks for your comment. Acctually it was also mentioned during our internal discusion. I forgot to add it. Let's call it:

1. move SCAN_RECORDER_MAP into /etc/ file (or other cofing file on Windows). In my opinion it also sounds good.

Cheers,
Jan

On 09/25/15 10:18, PICCA Frederic-Emmanuel wrote:

Hello, guys, I saw Debian so I jump in :)

If I understand correctly this is an extension mechanism in order to register new recorder into Sardana.
right ? via a third party module which could be provided by another package.

The usual way for a package to be extended by another one is to provide something like a
/etc/sardana.d/
directory where other packages can drop in there extensions configurations files.
by extension I mean a config file which is red by Sardana at startup and which can teach him where other extension are available.

so you extension package should install a private python module or a public one which and register it via this sort of mechanism.

Usually it is great also to provide a way for the local administrator to override thes package extensions.
A way to says this pacakage installe this recorder but I do not want on my site this functionnality.

A good exemple of this kind of configureation is the systemd config system. [1]
DEclarative with threee level of configuration, package, system administrator, user
The xdg config system can be usefull for this purpose. [2]

Another possibility is to propose an extension at the python level but this is the object of a dedicated PEP.

I hope thaht I did not misunderstood your problem.

Cheers

Fred

[Teresa Nunez]

Hi,
I would try to avoid any solution implying that the user has to know if
there is any other recorder with the same extension or any other knowlege
about the recorders in the system. So, I don't like the solution 4, the 2 would
be for me the best of the proposed ones. The 3 has the problem of duplicity
of extensions, and the 1 ... well, the users are used to extensions, I think it
is better to keep it like this and not forcing the users to define an environment
variable more.

                              Regards,
                                                 Teresa

[Zbigniew Reszela]

The feature is already implemented (thanks to Jan) and code review is completed.
Since in the code review process I had to change some implementation I let anyone interested to review the code and comment before the integration (code is available in branch feature-380-rebase in fork git://git.code.sf.net/u/cmft/sardana-GenericSoftware)
If no one comment, I will proceed with integration to develop on next Monday.

At Alba we are also planning to dedicate some time and provide a clear documentation on how to use the new feature (from the configuration and the end user point of view). Meanwhile I paste here a brief description of the final state of the feature:

Recorder classes are considered as Sardana plugins and are managed by the RecorderManager which belongs to MacroServer (see analogy to Macros).
Users can easily add custom recorders by placing them in one of the RecorderPath directories. On the MacroServer startup these directories are searched for valid recorder classes (the search process is not recursive). GScan macros e.g. dscan decides which recorders to use based on the ScanRecorder and ScanFile environment variables. The criteria is as following:

1. If ScanRecorder variable is defined (its type can be either a list of string, or just a string representing recorder class name(s)) it indicates which recorder class should be used and for which file defined by ScanFile.
   • Example 1:
     ScanFile = myexperiment.spec
     ScanRecorder = FIO_FileRecorder
     FIO_FileRecorder will write myexperiment.spec file.
   • Example 2:
     ScanFile = myexperiment.spec, myexperiment.h5
     ScanRecorder = FIO_FileRecorder
     FIO_FileRecorder will write myexperiment.spec file and NXscan_FileRecorder will write the myexpriment.h5 (the selection of the second recorder is based on the extension map - see point 2)
2. If ScanRecorder variable is not defined, recorders are selected based on the file extension specified in the ScanFile variable. Extension to recorder map is created when interpreting recorder classes located in both: the RecorderPath directories and the recorder catalog of Sardana (from now on we refer to both of them as recorder directories). The lower priority recorders are the ones from the catalog and the highest priority are the ones from the first directory defined in the RecorderPath.
   • Example 1:
     Extension to recorder map:
     { '.h5': ['NXxas_FileRecorder'],
     '.spec': ['SPEC_FileRecorder'],
     '.fio': ['FIO_FileRecorder']
     }
     ScanFile = myexperiment.h5
     NXxas_FileRecorder will write myexperiment.h5 file.
   • Example 2:
     More than one recorder implementing a given extension can be located in the most priority recorder directory. In this case Sardana will not execute the scan and ask the user to explicitly select the recorder using the ScanRecorder variable.
     Extension to recorder map:
     { '.h5': ['NXxas_FileRecorder', 'NXscan_FileRecorder',
     '.spec': ['SPEC_FileRecorder'],
     '.fio': ['FIO_FileRecorder']
     }
     ScanFile = myexperiment.h5
     Scan will not be executed and user will be asked to explicitly select the recorder.
3. If neither rule 1 nor rule 2 is fulfilled, the SPEC_FileRecorder will be used.

Other rules or use cases:

• We have also marked 'FileRecorder' macro hint as deprecated. We plan to maintain it (unitl further notice) as a valid criteria to select the recorder class and its priority is between rules 1 and 2.
• User may override a default recorder, defining one of the same name, and placing it in the RecorderPath directory. E.g. The OutputRecroder behavior may be modified this way.
• Users may overcome the situation defined in rule 2 and example 2, by specifying the selection with the SCAN_RECORDER_MAP defined in sardanacustomsettings.

And some implementation details:

• Final recorder classes were moved to a dedicated "recorder catalog". We will try to maintain the backwards compatibility by allowing imports of them from their original location e.g. from sardana.macroserver.scan.recorder.storage import SPEC_FileRecorder. But if it is not possible we will sacrify it on favour of feature-380 (it is very unlikely that anyone had used the final recorder classes in this way).
• Recorder manager handles all types of recorder: file recorders, output recorders, etc..
• RecorderManager was intentionally inspired on the MacroManager even knowing the limitations of the current implementation (see [#401] and [#402]).

[Zbigniew Reszela]

Finally it was not trivial to maintain backwards compatibility of the recorder imports from their previous location. Standard delaying of imports was not successfull, so I have evaluated the use of metaclasses.
See the attached example:

• module orig (new location) defines class A (original recorder class)
• module meta (old location) defines class B (duplicated recorder class just to maintain backwards compat.)

The idea was to delay imports of class A from within the module meta until the moment that it was really necessary e.g. instantiating a class, or checking if an instance is of the original class. These two cases works well thanks to use of metaclass. But without changing the original class (adding a metaclass to it) I could not achieve that subclasses of the duplicated class are considered as subclasses of the original one.

So finally I discarded this as valid option for maintaining backwards compatibility.

In the investigation I used this article:
http://blog.ionelmc.ro/2015/02/09/understanding-python-metaclasses/
and this python reference:
https://docs.python.org/2/reference/datamodel.html#customizing-instance-and-subclass-checks

An email will be issued to the users mailing list, notifying about breaking backwards comparibility with import of the following classes: JsonRecorder, OutputRecorder, SPSRecorder, ShmRecorder, FIO_FileRecorder, NXscan_FileRecorder, SPEC_FileRecorder, NXxas_FileRecorder and DumbRecorder

Imports which used to work:
from sardana.macroserver.scan.recorder import JsonRecorder

Should be changed to:
from sardana.macroserver.recorders import JsonRecorder

Or if used from inside of the MacroServer scope e.g. in a macro code, should be changed to:
self.macro_server.recorder_manager.getRecorderClass('JsonRecorder') # where self is a macro object

[Zbigniew Reszela]

Feature was integrated in develop