ISISComputingGroup / IBEX

Top level repository for IBEX stories
5 stars 2 forks source link

genie_python: Validate block names in load script #5321

Open DominicOram opened 4 years ago

DominicOram commented 4 years ago

As a user I would like genie python to validate on load_script that when I run the script what I am setting/getting are valid block names so that I don't make typos. MARI has already implemented something like this themselves using the simulation mode created in #5480 (e.g https://github.com/ISISComputingGroup/ibex_user_manual/wiki/Simulating-Scripts). Once done discuss with MARI as to whether they can remove their code.

Acceptance Criteria

John-Holt-Tessella commented 4 years ago

How is this different from 5083?

DominicOram commented 4 years ago

5083 has a very long list of somewhat vague acceptance criteria. This is a very specific case that will be much easier to implement.

DominicOram commented 4 years ago

As of #5480 the simulation mode does this (see https://github.com/ISISComputingGroup/ibex_user_manual/wiki/Simulating-Scripts). This ticket should be to incorporate this into load_script. MARI already does this in one of their instrument scripts. Once done inform MARI.

mducle commented 4 years ago

Tested with IBEX 7.1.0 (server version 7.1.0.15bc0cf) on MARI and works well. I've now removed the old MARI simscript code, except for a short function which automatically runs specific function from a script file directly after loading it.

kjwoodsISIS commented 4 years ago

Ticket re-opened. It has not been through the proper review process.

DominicOram commented 4 years ago

From discussion with @mducle this could be done in two ways: 1) During load_script run all loaded functions in simulation mode (https://github.com/ISISComputingGroup/ibex_user_manual/wiki/Simulating-Scripts). This is what MARI currently do but has some potential pitfalls if instrument scientists are not expecting it: 1) If the script contains something outside of genie_python or some infinite loops these will also get run 2) If the script is created for a different config than the current one it will throw errors 2) Examine the AST for block names when loading the script. This requires none of the user's code to actually be run but could potentially hit a lot of edge cases quite quickly.

mducle commented 4 years ago

@DominicOram

This is what I use on MARI:

import genie_python.genie
import os.path

def load_script(name, check_script=True, warnings_as_error=False):
    genie_python.genie.load_script(name, check_script, warnings_as_error)
    globs = genie_python.genie._get_correct_globals()
    if 'runscript' in globs and os.path.samefile(globs['runscript'].__code__.co_filename, name):
        with genie_python.genie.sim.Simulate():
            globs['runscript']()

It lives in the inst module and is invoked as inst.load_script instead of g.load_script.

We have a sort of "standard" where we write scripts with just a single function runscript and load and invoke that, so I just check for that specific definition and run it through the simulation environment instead of anything else. The current genie load_script function already gets a list of function names so you could just as easily simulate each function instead.

FreddieAkeroyd commented 3 years ago

regarding AST approach, random though that occurred to me in queue in supermarket, can we make use of some existing static analysis or code coverage tool? They will be analysing functions or paths to them, maybe we can leverage their output or part of their code/approach