Expose single protocol run to executor #865
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #865 +/- ##
==========================================
- Coverage 97.37% 97.32% -0.05%
==========================================
Files 115 115
Lines 8695 8645 -50
==========================================
- Hits 8467 8414 -53
- Misses 228 231 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
To make it (slightly) simpler, I would:
turning your example into: from qibocal import Executor
protocol = {"id": "flipping",
"operation": "flipping",
"targets": [0,1,2],
"parameters": {
"nflips_max": 20,
"nflips_step": 2,
"detuning": +0.1,
}}
executor = Executor.create([protocol], platform="dummy", output="FOLDER")
completed = executor.run_protocol("flipping", mode="acquire")This is just for interfacing purposes, saving the user some boilerplate and limiting the required understanding of Qibocal internals. |
|
Concerning the mode, currently, Qibocal is using: qibocal/src/qibocal/auto/mode.py Lines 4 to 8 in a668a08 However, we could use an auto = ExecutionMode.acquire | ExecutionMode.fit | ExecutionMode.reportwith all the bits set. For |
|
Apart from names and other interface suggestions, the current proposal looks essentially good: it should be minimal, but it should allow running a fully flexible calibration program. Of course, if you have many of these protocols (calibration programs, not individual routines) there will be repeated patterns (e.g. the dependencies) and we could abstract them in the framework. |
|
Thanks for the review @alecandido. |
Yes, that's definitely an option. Since the |
There was a problem hiding this comment.
(Very) minor further suggestions, but to me, it's perfect.
It should be non-breaking for the current way of using it (i.e. CLI + runcards), and it should allow running individual protocols.
The only thing missing is an example of how to use it, but given that is a non-breaking/minimal contribution you could even merge it like this, and contribute the example in a further PR, considering this just as a refactor that will allow that feature (and not a new feature to be documented).
I can write an example on how to use it. We have already a section in documentation on how to use qibocal as a library https://qibo.science/qibocal/stable/tutorials/advanced.html#how-to-use-qibocal-as-a-library, which was doing similar things in a more complicated way. At this point shall we remove it and modify this section with the new layout proposed here? |
|
In view of #866, I would suggest a minor API change (demo code): from qibocal import Executor, Routine
from qibocal.protocols.operation import flipping
flipping_params = {
"id" : "flipping initial guess",
"targets": [0,1,2],
"parameters": {
"nflips_max": 20,
"nflips_step": 2,
"detuning": +0.1,
}}
def acquire(data, ...):
"""Custom routine"""
return ...
my_routine = Routine(acquire)
routine_params = {"......."}
executor = Executor.create(platform="dummy", output="FOLDER")
for i in range(10):
completed = executor.run_protocol(flipping, flipping_params, mode="acquire")
flipping_params['parameters']['detuning'] += i
completed2 = executor.run_protocol(my_routine, routine_params, mode="acquire")Where:
|
Yes, and I'd move it out from "Advanced Examples", and rather make its own page under tutorials (just to avoid passing the idea that this is something you should beware of).
Currently, we're coupling report generation to the execution, though the two things are well separate. qibocal/src/qibocal/cli/report.py Line 65 in 77f7bd1 (that already has a pretty simple API), even dropping the executor argument, i.e. assuming that an executor already ran.What we will need instead will be a History, but I would make that optional (if possible), and try to deserialize the History from the path itself (the purpose of having a History anyhow is to make the input path optional, i.e. you could generate everything in memory, before dumping to disk).The only other executor attribute used is:qibocal/src/qibocal/cli/report.py Line 99 in 77f7bd1 but we can inherit that in the history.
This should be sufficient for In any case, I'd do this in a separate PR. |
andrea-pasquale
force-pushed
the
run_single_protocol
branch
from
cfba3fc
to
b232aa7
Compare
|
Since this is working, and #869's target is to provide a simpler syntax to use what's already here, I wonder whether we should aim to merge this now. I will re-review, but I've already approved it, and this PR is just exposing something that is already more or less there in Since it's at an internal stage, we can avoid documenting it as a public feature (the public interface will come with #869), but the improvement is simple and valid on its own. |
| @property | ||
| def _actions_dict(self): | ||
| """Helper dict to find protocol.""" | ||
| return {action.id: action for action in self.actions} |
There was a problem hiding this comment.
The other thing we may consider is to kill the .actions attribute of the Executor, and essentially kill Executor.load() as well, just keeping Executor.run() for the runcards.
I.e., .load() is there to load a runcard, and .run() is there to execute it (since if you don't have a runcard you would use a script, not running all routines in one shot). So, we have two methods that are always going to be called one after the other (Executor.load(runcard).run(mode)), thus it could be just one (Executor.run(runcard, mode)).
| executor = Executor.load( | ||
| runcard, path, targets=runcard.targets, platform=GlobalBackend().platform | ||
| ) | ||
| # produce html | ||
| list(executor.run(mode=ExecutionMode.report)) | ||
| # TODO: skip report in a proper way | ||
| list(executor.run(mode=[])) |
There was a problem hiding this comment.
However, to replace Executor.load() we'll need to be able to deserialize a History (i.e. let's use History.load(path)), and replace executor occurrences here with history, as proposed in #865 (comment).
But this starts being a separate PR.
runcards/actions_qq.yml
Outdated
| - id: resoantor_amplitude | ||
| operation: resonator_amplitude | ||
| parameters: | ||
| amplitude_step: 0.1 | ||
| amplitude_stop: 0.5 |
There was a problem hiding this comment.
comment the lines
|
@andrea-pasquale @Edoardo-Pedicillo should we then solve the conflicts and merge this PR? I know we are going to change it even further, in #870 and #869, but this PR is rather self-contained, despite introducing a temporary solution (in any case, |
Adds possibility to run one protocol at a time using
Executor.Possible script:
@alecandido I think this should be more or less what we discussed :)
Checklist:
mastermainmain