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 RecorderManagerCustomization 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 recorderAll 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 spocksenv 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
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!!.
Last edit: Carlos Falcon 2015-09-15
Thanks.
BTW, the last command should be
git checkout -b feature380 NAME_OF_YOUR_REMOTE/ feature-380
Hi,
Accidentally I push my develop and master branch into cmft.
Do you know How to revert it easily.
Best regards,
Jan
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
The correct http adress from previous is:
deb http://repos.pni-hdri.de/apt/debian wheezy main
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):
This way allows the user uses different default recorder per extensions.
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
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:
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)
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)
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.
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
Hello Fred,
Thanks for your comment. Acctually it was also mentioned during our internal discusion. I forgot to add it. Let's call it:
Cheers,
Jan
On 09/25/15 10:18, PICCA Frederic-Emmanuel wrote:
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.
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:
ScanFile = myexperiment.spec
ScanRecorder = FIO_FileRecorder
FIO_FileRecorder will write myexperiment.spec file.
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)
Extension to recorder map:
{ '.h5': ['NXxas_FileRecorder'],
'.spec': ['SPEC_FileRecorder'],
'.fio': ['FIO_FileRecorder']
}
ScanFile = myexperiment.h5
NXxas_FileRecorder will write myexperiment.h5 file.
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.
Other rules or use cases:
And some implementation details:
Related
OLD Tickets: #401
OLD Tickets: #402
Last edit: Zbigniew Reszela 2015-11-24
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:
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
Feature was integrated in develop