pre- and post-move hooks not available in mv macro

[dschick]

Hi all,

I have an issue with the pre- and post-move hook place which is similar to #770 .

In SPEC there are hook place like pre-acq/post-acq and pre-move/post-move. They are run everytime you do an acquisition (pre/post-acq) or a movement of a motor (pre/post-move) - independent if the parent macro is a scan or a ct or mv macro.

In Sardana was solved for ct with #808 but is not working for umv or mv.

I would suggest to add the hook places pre/post-move also to these macros as it is more consistent and our case also necessary.

Best

Daniel

[reszelaz]

Hello! For me it make sense to add these hook places. Do you plan to work on that? Thanks!

[dschick]

okay, if you agree I will prepare a PR

[reszelaz]

Let's wait a bit and see if there are other opinions.

[teresanunez]

Hi, Zibi, according to your answer I don´t know if you are aware that those hook places are there, but for scans, they are not called when the macros mv or umv are called. In fact I am not sure that it is a good idea to add them, In our case the hooks are really handling a lot of things exclusively needed for the scans, but not for simple movements of the motor. What could help to not disturb to our users is that I think they mainly use pre-acq and post-acq and pre/post-scan. I would say that pre/post-move are less used and do not make the whole scan staff ... But in any case, running Hooks every time a single motor is moved or having to deactivate them before making these single movements ... I do not think that it is a good idea. Perhaps there is an alternative to make it optional or to some how define another hook place or flag for the single movements. Best Regards, Teresa

[jangarrevoet]

I agree with Teresa. I am using these hooks and really see these hooks as "scan hooks". Since a move is not a scan I would strongly oppose since this will potentially break a lot of peoples code.

[reszelaz]

Thanks @teresanunez and @jangarrevoet for raising the question of backwards compatibility. I perfectly understands and agree with your concerns.

But putting them apart for a moment, do you agree that having a pre and post move hook places in all the generic macros involving movement would be more consistent? If one would like to configure and implement specific hooks only for scans then in the code of the hook would need to check if the parent macro is a scan. This information is already available in Macro.hints e.g. https://github.com/sardana-org/sardana/blob/c646206b3f532e717be073c4c01e33a6e6358d75/src/sardana/macroserver/macros/scan.py#L119

Also, as @dschick already mentioned, we made a similar decision for the ct and uct macros and pre and post acquisition hooks.

If we finally agree about adding this hook places to the mv and umv macros we could discuss the backwards compatibility. I see the following options:

1. Do nothing and just advice to use:

   @macro()
   def my_hook():
       if self.parent_macro.hints.get("scan", None):
           # here place the original code of the hook

   in the already existing hooks if the hook can be problematinc if also executed in mv or umv. We could even think of adding a more direct API e.g.

   
   @macro()
   def my_hook():
       if self.parent_macro.isScan():` 
           # here place the original code of the hook

2. Add e.g. MV_HOOKS_IN_MV_MACROS=True in sardanacustometting meaning that these new hook places will take effect by default but could be disabled with MV_HOOKS_IN_MV_MACROS=False.
3. Add e.g. MV_HOOKS_IN_MV_MACROS=False in sardanacustometting meaning that these new hook places won't take effect by default unless one enables them with MV_HOOKS_IN_MV_MACROS=True.

[teresanunez]

Hi Zibi, I would not implement any solution implying some action from our users. They use constantly the hooks for the scan since years and it will be really a problem if we now force them to make any change. The solution 3 would be the only acceptable one, if this is needed, for the other ones I think we would make again a change in the sardana code installed at DESY.

                              Best Regards,
                                                          Teresa

[teresanunez]

Hi Zibi, and defining a new hook place ?, or do you want that always the same running for the scans without any extra setting would run?.

[reszelaz]

Hi Teresa, IMHO defining a dedicated hook places (with different name as pre-move or post-move) for mv and umv macros is not necessary. If I think about a new Sardana user who just start programming hooks, adding an extra line in the hook: if self.parent_macro.isScan(): looks reasonable to me in case he/she would like to execute some actions only in the case of scans.

Of course there is a backwards compatibility issue we need to consider. I understand that option 3 would be the most convenient to you. Let's dedicate some time on this Wednesday's follow-up meeting and brainstorm about it...

When I was thinking about the backwards compatibility layers 2 and 3 I was also thinking about adding a deprecation warning in case these hook places are disabled.

[teresanunez]

Hi ZIbi, there is also another point that we have not taken into account. Any macro can be used as hook, if I use as hook an standard sardana macro I can not add to it the if statement, this would apply only to user defined macros, or in other case it will be allways called.

[reszelaz]

Any macro can be used as hook, if I use as hook an standard sardana macro I can not add to it the if statement, this would apply only to user defined macros, or in other case it will be allways called.

Yes, you are right. In this case the only possibiliy would be to implement a custom hook that would call the standard macro based on the condition. Still I do not see it terrible. But I do not have too much feeling on how the users use the general hooks - we still do not use them so ofter at ALBA. Just to clarify it to others who may not have it clear, all the problem applies only to the general hooks. The other hooks are always explicitly attached to a given hookable macro. Could you share here some examples on how you use the pre-move and post-move general hooks?

[teresanunez]

Hi Zibi, thanks. Perhaps @jangarrevoet put give some example, I do not want to publish code that is not mine.

[jangarrevoet]

I do some quality checks in certain scans and it can also be linked to an optical camera that acquires data but is not linked to the acquisition part of the scan. I would in general say that giving them new names would be less confusing and safer. If people want to have the same functionality they just set up the hooks to execute the same code, which will be set and forget. In this way you do not have to worry about backwards compatibility and actually add a new capability without any downsides.

[reszelaz]

Thanks @jangarrevoet for explaining your case!

We discussed this issue during the last follow-up meeting and we decided the following:

1. We will add pre-move and post-move hook places to the mv and umv macros and future Sardana built-in macros should provide the same hook places. This is already proposed in #1471.
2. We plan to add a possibility to define General Hooks on similar basis as the environment variables e.g. on the global, door and macro level. We also plan to extend the environment variable definition on the macro family level e.g. scans would be one family. Then, it would be also possible to define the General Hooks for the family of macros e.g. scans.
3. Meanwhile the point 2 is discussed and implemented, we will provide a sardanacustomsetting to enable/disable the hook places in the mv and umv macros.

Regarding point 3 on the follow-up meeting we said to have these hook places disabled by default. After the meeting I was thinking about it again, and IMHO for the new users of these hook places it would be better to start designing/implementing hooks with the behavior that we agreed and want - any build-in macro move will be surrounded by pre-move and post-move hook. Otherwise more users will need to change the configuration in the future whenever we add point 2.