search

sardana-org / sardana

Moved to GitLab: https://gitlab.com/sardana-org/sardana
39 stars 52 forks source link

Hooks by default in scans (SF#354) #200

Closed sf-migrator-bot closed 6 years ago

sf-migrator-bot commented 9 years ago

Hi, I copy here an e-mail that I sent some time ago, it has become now a request from our users.

our beamline scientists want that all the scans run by default with hooks functions, not like it is now that it is necessary to define a new macro running an existing scan macro after adding hooks to it. They want to keep the original names for the current scan macros and simply define a function that will be run always like a hook. I think that the current implementation for the hooks works quite well, and offers flexibility for example for the arguments that have to be passed. A fixed function could have some problem there. So in my opinion one should keep what is implemented and try to add the requested feature that could be activated by an additional variable (set for example in the spock environment). What do you think about something like this?. 

                        Regards,
                                   Teresa

Reported by: teresanunez ( http://sf.net/u/tere29 )

Original Ticket: sardana/tickets/354

sf-migrator-bot commented 9 years ago

Hi again, only for avoiding parallel work in the same subject ... I have implemented a first approach for getting this done. The user can simply define some functions (with fixed names) and they will be run during the scan (like the hooks: pre-move, pre-acq, ...), even if these functions are of course not macros, one can have the macro available inside, so one can profit from all the advantages that this implies. It will need for sure some cleanup and some more details ... but perhaps it could be used as start point.

                   Regards,
                               Teresa

Original comment by: teresanunez (http://sf.net/u/tere29)

sf-migrator-bot commented 9 years ago

looks nice. Just one question: once a hook function is defined (e.g. pre-move) how do you control which macro uses it and which doesn't? (imagine I want ascan to use it but I do not want mesh using it)

One possible way would be to use environment variables (which already allow for several levels of scopes). And as a bonus, you do not need to hardcode the names of the functions, because the value of the variable could be a dictionary mapping hook names with function and/or macro names.

For example:


# Assume m1, m2 are macros defined somewhere in the macropath
# Also assume that myhooks.py is a module located 
# somewhere in the pythonpath and which defines
# functions called f1, f2, ...

# set the macros m1 and m2 as the pre-move hooks for ascan only
senv ascan.HOOKS "{'pre-move':['m1', 'm2']}"

# set f1 as the post-acq hook for all macros that use post-acq hooks
senv HOOKS "{'post-acq':['myhooks.f1']}"

# set f2 as the pre-move hook for ascan only
senv ascan.HOOKS "{'pre-move':['myhooks.f2']}"

# set both f3 and f4 as the pre-ack hooks for all macros on MyDoor
senv MyDoor.HOOKS "{'pre-move':['myhooks.f3','myhooks.f4']}"

A further sophistication would be to accept args and kwargs using an optional length-3 tuple notation such as:

# assume f5 accepts two args plus the x and y kwargs:
senv HOOKS "{'post-acq':[('myhooks.f5', (1,2), {x=3,y=4})]}"

# assume we want to always move mymotor to 0 before all scans
senv HOOKS "{'pre-scan':'[('mv', ('mymotor 0'), {})]}"

Original comment by: cpascual (http://sf.net/u/cpascual)

sf-migrator-bot commented 9 years ago

Hi Carlos, many thanks for your comments and suggestions. I didn't want to read much more from the environment, but I will test it, because I think it is really a good idea to make it like this. Many thanks.

                   Regards,
                              Teresa

Original comment by: teresanunez (http://sf.net/u/tere29)

sf-migrator-bot commented 9 years ago

Hi Teresa,

We would like to have a look to the DESY solution to the hooks in order to be able to discuss about this subject during the Sardana Workshop. Could you please push your implementation to a remote branch where we could have a look to it?

We would like to ask you if you would like to present the 'hooks' during workshop at DESY. In this case it would be nice to present both ideas, the DESY implementation and the Carlos idea about using Sardana environment variables. What do you think?

Regards, marc

Original comment by: sagiss (http://sf.net/u/mrosanes)

sf-migrator-bot commented 9 years ago

Hi Marc, I will try to put the implementation in some branch, I have first to make some changes in my files ... I mean, go back to the versions in the sardana develop branch (we have some changes at DESY), but I hope I will manage in the next days. Of course, I can present something about the hooks, in fact there is already a point in the Workshop program about that.

                       Regards,
                                          Teresa

Original comment by: teresanunez (http://sf.net/u/tere29)

sf-migrator-bot commented 9 years ago

Hi Marc, for the implementation of the general hooks I had modified only one file (gscan.py). I have just now taken the current version of this file from the sardana develop branch and applied the changes. I send you the file per e-mail with documentation how to used (I tried to put the link here, but it doesn't work). Tell me if you have problems trying it. Many thanks.

                   Regards,
                                        Teresa

Original comment by: teresanunez (http://sf.net/u/tere29)

sf-migrator-bot commented 9 years ago

The file with the implementation

Original comment by: teresanunez (http://sf.net/u/tere29)

sf-migrator-bot commented 9 years ago

Documentation how to use it:

http://hasyweb.desy.de/services/computing/Spock/

Section:

Scans -> Hooks -> General hooks, conditions and on_stop function

Original comment by: teresanunez (http://sf.net/u/tere29)

sf-migrator-bot commented 9 years ago

After analyzing the Teresa's solution, IMHO it is not generic. It uses a specific file that it should be on the PYTHONPATH of the MacroServer PC and the order of the hook execution could be important. I agree with the Carlos's solution (to use the environment), because it is independent of any file, but the current way to edit the environment is not user-friendly (too complicated).

In the meantime, we can find an alternative solution to set the hook in the environment, e.g: with a specific GUI (similar to expconf) or with specific macros. Another solution is to set an environment variable to specify the file with an API (to be develop) to configure the hooks. If we want a complex solution but generic, we can create a HookManager which is responsible to manage the different entry points (file, function, environment, etc.).

Original comment by: rhomspuron (http://sf.net/u/rhomspuron)

sf-migrator-bot commented 8 years ago

Hi Roberto, many thanks for your comments and sorry for not answering until now, I did not have any time for looking at it again. Here some replies to your comments:

'It uses a specific file that it should be on the PYTHONPATH of the MacroServer PC'

It is not in the PYTHONPATH, we add the path to the PythonPath property of the MacroServer device. About the MacroServer PC, well, this is the case for all the files used by the MacroServer, included Macros or any external module.

'the order of the hook execution could be important'

I don't know what do you mean with that. The order is und has to be important.

' I agree with the Carlos's solution (to use the environment), because it is independent of any file, but the current way to edit the environment is not user-friendly (too complicated).'

We can handle this with macros, I think this is not the problem, the main problem for us was the overloading of the environment and that the users do not like the idea of using it. But well, it can be done with macros and keep transparent for the users that everything is in the environment ... even if they will see it ... I don't think that GUIs are a good idea, for being used at DESY, I mean. But how to set the environment is independent of the implementation, every one could use like it is prefered. Should I try to change the current implementation following the hints from Carlos or do you prefer to discuss before any more possible scenarios, implementations ...?.

                     Regards,
                                             Teresa

Original comment by: teresanunez (http://sf.net/u/tere29)

sf-migrator-bot commented 8 years ago

Teresa, I think that the solution with the environment is more flexible and more standard. It would be great if you could modify your solution and do it this way.

I think that we should agree what to do in the case where macro has explicitly defined hooks in its code. I see the following options:

Personally I like the first option.

Regarding the environement variable name, I would use the CamelCase writing and call it Hooks. What do you think?

Original comment by: reszelaz (http://sf.net/u/zreszela)

sf-migrator-bot commented 8 years ago

Hi Zibi, many thanks for your commets. I will work on the change of the implementation. Abot the options you list, here we use the solution 4:

execute all hooks, first the ones from the environmnet and then the ones from the macro

For us the discussion was between the third and the fourth you list, we want our general hooks to be executed always, but in addition we can add specific hooks in the own code of a given macro. We can not work with the first solution. We have macros with own hooks defined, they users could not know about that, and they want in any case that the general hook will be always applied, also to these macros. If you don't agree with this option, we can make it selectable.

                  Regards,
                                          Teresa

Original comment by: teresanunez (http://sf.net/u/tere29)

sf-migrator-bot commented 8 years ago

Initially I was thinking that the explicit definition of the hooks in the code should be the only one executed. But as you say, from the user point of view it will be hard to know about the implementation details. I agree with you, but let me ask here at Alba... We will reply in this thread if there are some arguments agains option 4.

Original comment by: reszelaz (http://sf.net/u/zreszela)

sf-migrator-bot commented 8 years ago

Hi, I have just sent a patch to the mail list with an implementation of the general hooks based on a dictionary enviroment variable called GeneralHooks. The implementation allows the use of macros, with any argument also given in the same environment variable, as proposed by Carlos, as general hooks. We have decided that this is enough and even better for us. Our general functions were passing always the macro as argument, because some features of the macros, like execMacro or the spock output were usually needed, so it is better that they are already macros. Please have a look to the implementation and send your comments. The environment variable can be set directly from spock like:

senv GeneralHooks {\"pre-scan\":[(\"mv\" \"exp_dmy01\" 50)] \"pre-move\":[(\"mv\" \"exp_dmy02\" 50)]}

But I will make macros for setting everything via Macros, so that the users do not have to take care of this complicated syntax. We will also probably fix the name of the macros for being run as general hook at each point in our spock environment, making everything even easier for the users, but this is something that has nothing to do with the implementation. I have not done anything for the continuous scans, since they are changing considerable in the SEP6. But it also could be extended if needed. I will also attach the patch to this ticket.

                                  Regards,
                                                         Teresa

Original comment by: teresanunez (http://sf.net/u/tere29)

sf-migrator-bot commented 8 years ago

Here the patch.

Original comment by: teresanunez (http://sf.net/u/tere29)

sf-migrator-bot commented 8 years ago

Hi, I have also changed the implementation of the general condition for checking if a point of the scan should be repeated in the same way, using another environment variable that should be an string with the macro to be run for checking (also arguments can be given). I do not send the patch, because in any case for the discussion is enough to see the implementation of the general hooks.

                          Regards,
                                           Teresa

Original comment by: teresanunez (http://sf.net/u/tere29)

sf-migrator-bot commented 8 years ago

Hi, in the same way I have also changed the implementation for the general stop function. The difference with the two implementations above is that in this case it has to be a function (a macro is not posible because with ctrl-C we stop all macros, so call to macro members like execMacro does not work any more). I think for the general hooks and condition we can keep only macros, but if you want to extend it to functions, it could also be done. In my opinion is not needed, and I say in some previous mail, the macro was even always sent as an argument to the function because it was need it.

                       Regards,
                                            Teresa

Original comment by: teresanunez (http://sf.net/u/tere29)

sf-migrator-bot commented 8 years ago

Hi, I would like to know your opinion about one point. What should happen if an execution of a general hook macro fails or if the macro is not found?, and the same question for the general condition and stop function. In the patch I have sent the scan will be stopped if any general hook fails. But I think it would be better if the scan ignores this and it continues. For the general stop, for example, I already implemented ignoring the error ang giving a warning. I will change the implementation of the general hooks and conditions in the same way. In the general condition, if the check of the condition fails the point will not be repeated and a warning will be printed out. Do you agree with this?.

               Regards,
                                    Teresa

Original comment by: teresanunez (http://sf.net/u/tere29)

sf-migrator-bot commented 8 years ago

Hi Teresa, I was about to integrate this feature, but I realized that you mention some other changes that are not sent.

Could you please send all the patches so that I integrate it all?

Original comment by: cpascual (http://sf.net/u/cpascual)

sf-migrator-bot commented 8 years ago

Hi Carlos, I send a patch with the last implementation. I have also made some macros to set the environment, they have still with a preliminary name to not conflict with the old implentation I did, which is still used in some beamlines. I send also the file with the macros and some info, this helps to know how to use the implementation:

-- new_gh_enable -> enable hooks, if not argument is given enable all hooks with default values: 'pre-scan': ['gh_pre_scan'], 'pre-move': ['gh_pre_move'], 'pre-acq': ['gh_pre_acq'], 'post-acq': ['gh_post_acq'], 'post-move': ['gh_post_move'], 'post-step': ['gh_post_step'], 'post-scan': ['gh_post_scan']

For giving other names, ex. for a post-scan hook:

new_gh_enable "mymacroforpostscan oneargument otherargument" post-scan

or several macros:

new_gh_enable "mymacroforpostscan arg1 arg2, myseconmacro arg3" post-scan

-- new_gh_disable -> disable all general hooks

-- new_gh_isEnabled -> return true and macro names if general hooks are enabled.

new_gc_enable macro_withoutargs

new_gc_enable "macro_withargs arg1 arg2"

Only one macro is allowed, since a condition is checked and it is better to have all the logic in one macro and not start making 'and'/'or' conditions betweenn different macros.

-- new_gc_disable -> disable general condition

-- new_gc_isEnabled -> return true and name of the macro if the general condition feature is enabled.

The 'new' in the name of the macros will disapear, I only keep it like this for the moment for not having conflicts with the previous implementation.

                                       Regards,
                                                              Teresa

Original comment by: teresanunez (http://sf.net/u/tere29)

sf-migrator-bot commented 8 years ago

The patch

Original comment by: teresanunez (http://sf.net/u/tere29)

sf-migrator-bot commented 8 years ago

Hi, I haven't finished the review yet (I just reviewed the code, but didn't do any tests). But there are some comment already:

I am aware that some of my proposed changes may break your existing workflow, so just take them as a proposal to find a solution that is ok for all.

Original comment by: cpascual (http://sf.net/u/cpascual)

sf-migrator-bot commented 8 years ago

Some more comments.

In your patch you create a mechanism for executing the "general hooks" which is parallel to that of the normal hooks. Instead, I would prefer to extend the original mechanism to support the definition of hooks via the envvar. (i.e. merging macro.getGeneralHooks and macro.getHooks, and then also extend the part of gscan where the hooks are executed)

Also, for API simplicity, the onStopFunction could be implemented as just another hook. AFAIK, the stop function is similar to the "post-scan" hint with maybe the difference that it is executed no matter what (i.e. it would be executed in the "finally" clause of a try-except-finally construction... in fact maybe it could just be a hook with "hint='finally'" )

Original comment by: cpascual (http://sf.net/u/cpascual)

sf-migrator-bot commented 8 years ago

Original comment by: cpascual (http://sf.net/u/cpascual)

sf-migrator-bot commented 8 years ago

Hi Carlos, we want to have the two different implementations, the hooks called from the macro in which you have defined them, and the hooks that are general and then affecting all the macros. Do you mean to define only one function but making what now make getGeneralHooks and getHooks exactly like it is done now?. I don't see your last point, sorry. The use of the onStopFunction should not imply any extra change in any macro, adding any kind of hint or function to it. It has to be only a function that will be executed any time and only when a ctrl-c is done. I have tried to keep the current implementation without the minimum modifications of Sardana (mainly because we are already using it and I have to keep the changes every time we take again the develop branch from Sardana). If you prefer to make a major change, no problem. Again thanks for the review.

                         Regards,
                                              Teresa

Original comment by: teresanunez (http://sf.net/u/tere29)

sf-migrator-bot commented 8 years ago

Hi Carlos, many thanks. I add hier the answers again. Point 1. You have already found it. Point 2. I did not put the functions because it does complicate the implementation without gain. I think it is cleaner and easier to use only with macros. Our users prefer now this implementation, and we also prefer to give them a way to do it without too much thinking between different possiblities. In the previous implementations we had only functions and it was much more confussing to use.
Point 3. No problem, I put it like this again for simplicity (in this case for the users). The general on_stop can not be a macro, but well, if we want to put the hooks and conditions also as functions, will be a way of differentiating. Point 4. If you suggest another way, no problem, it was the only way I found for being sure that the modules are always reloaded when the functions are called, without the users having to take care of that.

                                  Regards,
                                                         Teresa

Original comment by: teresanunez (http://sf.net/u/tere29)

sf-migrator-bot commented 8 years ago

Original comment by: reszelaz (http://sf.net/u/zreszela)

sf-migrator-bot commented 8 years ago

Hello, I have slightly changed the implementation for passing the Interrupt signal in case ctrl-c is done during the scan (in the files I sent, the scan will not be interrupted if the post-acq general hook is activated). Since the implementation has still not been accepted I do not send a new patch again, but if this performance is checked only let you know that the problem is solved in the last version.

                                  Regards,
                                                     Teresa

Original comment by: teresanunez (http://sf.net/u/tere29)

sf-migrator-bot commented 8 years ago

Original comment by: reszelaz (http://sf.net/u/zreszela)

sf-migrator-bot commented 8 years ago

I have reviewed the whole thread and the code and below are my comments:

  1. I am also in favor of exposing to the macro developer only getHooks method - avoid adding getGeneralHooks to the API (But if you see a use case extending the hooks concept - having general hooks, programmed hooks, XML hooks, ... we could also discuss that...). I think it will simplify and unify the hooks usage in Sardana. I see these complex scenarios of usage:
    • If one uses hooks in the sequencer, getHooks should return the general hooks and the hooks attached in the XML
    • If one uses hooks programmatically, getHooks should return the general hooks and the hooks attached programmatically
    • I think that an error during the hook execution should break the macro execution. The same if the configuration is wrong e.g. hook macro does not exist.
    • I think that the process of extracting the hooks from the environment should be implemented and hidden in the sardana.macroserver.msmacromanager.MacroExecutor class. A very similar process is already implemented for the hooks defined in the XML (see method _prepareXMLMacro).
    • The macro hooks should be implemented similarly to RunSubXMLHook which instances could be easily called. Hence the macro programmer could directly execute: 
      :::python
for hook in self.getHooks('my-hook-place'):
hook()

In order to continue this development I propose the following:

  1. I created a remote branch so it will be easier to coordinate development and testing: .
    • We should first focus on the core implementation (basically until the getHooks return the correct hooks). In order to test that I have prepared a simple module that should be extended with more use cases .
    • Afterwards we could work on the user friendliness of the configuration macros, etc.

What do you think if in order to facilitate the development, and its integration in the official Sardana we do it incrementally e.g.

  1. Implement general hooks as macros only.
    • Focus on the general on_stop hook e.g. verify if not being able to execute macros in on_stop is a limitation (maybe we could overcame it..) - it could be related to [#9]
    • Implement general hooks as functions (if needed for on_stop).
    • Focus on the on general condition feature.
    • Implement general conditions as functions.

Original comment by: reszelaz (http://sf.net/u/zreszela)

sf-migrator-bot commented 8 years ago

Hi Zibi, many thanks for reviewing and for your comments. I agree with the way of proceeding that you propose. Do you plan to apply the changes or do you want me to start with them?. We are not in a hurry, since our users are using the current implementation since months and they are happy with it, so they can continue like this until a new one is ready. But it is better if you have as soon as possible a common solution integrated in sardana. Just tell me if I should start. Regards, Teresa

Original comment by: teresanunez (http://sf.net/u/tere29)

sf-migrator-bot commented 8 years ago

Original comment by: reszelaz (http://sf.net/u/zreszela)

teresanunez commented 7 years ago

Hi Zibi, I have read all that again and I think we have a different idea of what a general hook is. For us a general hook will be never called or set by the programmer in any other macro, for that we have already the standard hooks. This was exactly the reason why we have implemented the general hooks, they are something that applies always and nothing have to know or program anything extra in any other macro. About breaking the scan if there is an error in the general hook, we have discussed this point with our users, and they want the scan to continue and get only a warning about the execution of the general hook. Perhaps one can implement the possibility of choosing what to do. We would also prefer not to give too much flexibility in choosing between macros or functions for the general hooks. We have tested both cases and the users are more happy with macros (except for the case of the on_stop function, since no macro can be executed after a control-C, but I am not sure if this has been changed recently). So I do not really know how to proceed with that for getting it integrated.

reszelaz commented 7 years ago

I have some comments regarding the GeneralHooks syntax proposed in https://github.com/sardana-org/sardana/issues/200#issuecomment-262975158. The proposed general hook map uses the hook place name as the key and the list with macro(s) to be executed at that hook place as values. This syntax is somehow opposite to what we currently have for setting hooks programmatically.

See documentation: https://github.com/sardana-org/sardana/blob/01b154244c97e09b316be39fbf2e7a15d149e4f3/src/sardana/macroserver/macro.py#L210 And example: https://github.com/sardana-org/sardana/blob/01b154244c97e09b316be39fbf2e7a15d149e4f3/src/sardana/macroserver/macros/examples/hooks.py#L161 If we assume that this map is an internal data structure not intended to be directly set by the user it is fine. But then, when providing the user macros for dealing with the general hooks, IMHO, we should keep the syntax already used in the programmatic way. In other words, if the user would like to set all general hooks in a one shot, the syntax should use the macro as the key and the list of hook places as the values.

Apart from the fact that this is more coherent with what we have now, if the same macro has to be called at more than one hook place it avoids duplication of configuration. By the way, setting all general hooks in one shot is rather a complicated operation for the user, so I would also provide macros for adding partial configuration.

reszelaz commented 7 years ago

From the implementation point of view it would be much easier to use the internal data structure of GeneralHooks environment variable in the same form as it is already used in the programmatic way of setting hooks.

cpascual commented 6 years ago

I agree 100% in the value of keeping the environment syntax closer to the programmatic one.

But, on the other hand, note that the keys of a dictionary must be hashable, while the values can be anything. In the syntax I proposed this is granted since the hook-place names are hashable. And therefore one can:

I suppose that the first thing could also be accomplished in the "closer-to-programmatic" syntax by allowing the user to provide the name of a function (including its module), but support arbitrary args and kwargs would be a lot more difficult.

With this I am not saying that we should use my proposed syntax. I am only noting this limitation for all to be aware when evaluating the best solution.

reszelaz commented 6 years ago

The current programmatic API is based on the Hookable class which comprises a property called hooks. This property is a list of lists/tuples. Each of the internal lists has two elements, the first one is the hook itself and the second one is the list of hints, now used as hook places. It works with callables (arguments are currently not supported) and macros via the https://github.com/sardana-org/sardana/blob/01b154244c97e09b316be39fbf2e7a15d149e4f3/src/sardana/macroserver/macro.py#L255 helper class (parameters are supported). So, if we would like to maintain this format, we won’t be affected by the “unhashable key” problem mentioned in https://github.com/sardana-org/sardana/issues/200#issuecomment-343838853.

cpascual commented 6 years ago

Seems like a good option to me