FIT mode now creates platform with no hardware memory. - #1624
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1624 +/- ##
=======================================
Coverage 91.41% 91.41%
=======================================
Files 148 148
Lines 11841 11851 +10
=======================================
+ Hits 10824 10834 +10
Misses 1017 1017
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
RoyStegeman
left a comment
There was a problem hiding this comment.
Some comments. The main one being a bug the line below: while running qq fit, dummy_hardware will be True instead of None.
platform = create_dummy() if dummy_hardware is None else create_platform(platform_name)| from qibolab._core.dummy.platform import create_dummy | ||
| from qibolab._core.platform.platform import PARAMETERS |
There was a problem hiding this comment.
This is not part of the public API, meaning qibolab can change this without warning, so we should not use it in qibocal
There was a problem hiding this comment.
Ok I'll find a solution.
There was a problem hiding this comment.
in order to address this point, I opened Qibolab #1535 and then corrected the import in e47d647.
| rather than the platform definition. If a ``platform_name`` is provided, the hardware | ||
| configuration is loaded from that platform; otherwise, a dummy hardware | ||
| configuration is used so that acquisition-related fields are still present |
There was a problem hiding this comment.
If a
platform_nameis provided, the hardware
configuration is loaded from that platform; otherwise, a dummy hardware
is used
According to this the boolean dummy_hardware is not needed, but depends on whether a platform_name is provided. Although platform_name can't be None, so the docstring is not very clear.
|
|
||
| The platform is rebuilt from the configuration saved in the experiment history, | ||
| using the ``parameters.json`` and ``calibration.json`` files stored in the data folder | ||
| rather than the platform definition. If a ``platform_name`` is provided, the hardware |
There was a problem hiding this comment.
I think by "the platform definition" you mean the QIBOLAB_PLATFORMS environment variable. If so, please write that. If you meant something else, please clarify.
| create_dummy() if dummy_hardware is None else create_platform(platform_name) | ||
| ) | ||
| platform.parameters = parameters | ||
| platform.name = platform_name |
There was a problem hiding this comment.
Why do we need to overwrite the name?
There was a problem hiding this comment.
That action is only effective when platform is a DummyPlatform, I've decided to overwrite to propagate the name even for dummy platforms, maybe you need later in the code and I didn't want to create confusion by printing dummy (or whatever name DummyPlatform has).
|
|
||
|
|
||
| def check_overlap_in_input_qubits(targets: np.typing.ArrayLike): | ||
| def check_overlap_in_input_qubits(targets: list): |
There was a problem hiding this comment.
The first thing this function does is call targ = np.asarray(targets) so the previous type should have been correct.
There was a problem hiding this comment.
it's true, but also we call this function after casting self.targets as Target types. which indeed is always a list.
I assumed a more strict typehint since still list is an ArrayLike.
| ) | ||
|
|
||
| platform = ( | ||
| create_dummy() if dummy_hardware is None else create_platform(platform_name) |
There was a problem hiding this comment.
| create_dummy() if dummy_hardware is None else create_platform(platform_name) | |
| create_dummy() if dummy_hardware is True else create_platform(platform_name) |
Or maybe if platform_name is None; see comment.
| else CalibrationPlatform.from_datafolder( | ||
| folder_path=output, | ||
| platform_name=self.platform.name, | ||
| dummy_hardware=False, |
There was a problem hiding this comment.
| dummy_hardware=False, | |
| dummy_hardware=True, |
I think? Since in this case you're just fitting.
There was a problem hiding this comment.
yes thanks for pointing out
| if self.path is not None: | ||
| self._data.save(self.path) | ||
| self._data.save(self.path) | ||
|
|
||
| def dump_results(self): | ||
| """Dumping results.""" | ||
| if self.path is not None: | ||
| self._results.save(self.path) | ||
| self._results.save(self.path) |
There was a problem hiding this comment.
self._data and self._results can still be None, in which case calling None.save will raise an error.
There was a problem hiding this comment.
yes you are right, thanks
|
The proposal is fine by me. Possibly, we just want to eventually exclude platform access from the
This is a more complicated situation. The original design was meant to avoid passing paths everywhere, since disk I/O can be quite annoying in a HPC-like infrastructure. Because of the non-locality of the storage, and also the recommended folders access. However, we may even forget this part now, and decide to always work with paths. This happened accidentally, because the original design was violated (following the frequent quick & dirty approach), but we could even decide to systematically reconsider the initial assumptions. Whatever it is done here it is fine and acceptable. But let's keep this in mind, and redesign together. |
| @@ -25,15 +25,21 @@ def update(path: pathlib.Path, skip_qubits: list[QubitId] | None): | |||
| platform_name = json.loads((path / META).read_text())["platform"] | |||
|
|
|||
| platform_path = locate_platform(platform_name) | |||
There was a problem hiding this comment.
(qpu165) roy.stegeman@dalma:~/calibration$ qq update /home/users/roy.stegeman/calibration/results/tuna5/260804/17-19-51-cryoscope-0
Traceback (most recent call last):
File "/nfs/users/roy.stegeman/.venvs/qpu165/bin/qq", line 8, in <module>
sys.exit(command())
File "/nfs/users/roy.stegeman/.venvs/qpu165/lib/python3.10/site-packages/click/core.py", line 1514, in __call__
return self.main(*args, **kwargs)
File "/nfs/users/roy.stegeman/.venvs/qpu165/lib/python3.10/site-packages/click/core.py", line 1435, in main
rv = self.invoke(ctx)
File "/nfs/users/roy.stegeman/.venvs/qpu165/lib/python3.10/site-packages/click/core.py", line 1902, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
File "/nfs/users/roy.stegeman/.venvs/qpu165/lib/python3.10/site-packages/click/core.py", line 1298, in invoke
return ctx.invoke(self.callback, **ctx.params)
File "/nfs/users/roy.stegeman/.venvs/qpu165/lib/python3.10/site-packages/click/core.py", line 853, in invoke
return callback(*args, **kwargs)
File "/nfs/users/roy.stegeman/github/qibocal/src/qibocal/cli/_base.py", line 145, in update
updating(folder, skip_qubits)
File "/nfs/users/roy.stegeman/github/qibocal/src/qibocal/cli/update.py", line 27, in update
platform_path = locate_platform(platform_name)
File "/nfs/users/roy.stegeman/github/qibolab/src/qibolab/_core/platform/load.py", line 81, in locate_platform
return _search(name, paths)
File "/nfs/users/roy.stegeman/github/qibolab/src/qibolab/_core/platform/load.py", line 56, in _search
raise ValueError(
ValueError: Platform tuna5 not found. Check $QIBOLAB_PLATFORMS environment variable.
Sine no path is passed to locate_platform it will _search in the default _platforms_paths(), which is still
PLATFORMS_PATH = "QIBOLAB_PLATFORMS"
There was a problem hiding this comment.
Pull request overview
Reconstructs platforms from saved experiment data for FIT-only workflows, avoiding live hardware state.
Changes:
- Adds platform reconstruction from serialized parameters and calibration.
- Makes task output paths mandatory.
- Updates execution and fitting flows to use stored experiment state.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/qibocal/calibration/platform.py |
Adds data-folder platform reconstruction. |
src/qibocal/auto/execute.py |
Selects live or saved platform by execution mode. |
src/qibocal/auto/output.py |
Uses saved platforms during offline fitting. |
src/qibocal/auto/task.py |
Requires task paths and validates fitting data. |
src/qibocal/auto/history.py |
Makes generated task paths mandatory. |
src/qibocal/auto/runcard.py |
Uses the executor’s configured output path. |
src/qibocal/cli/update.py |
Loads updated platforms from serialized output. |
tests/test_output.py |
Adapts output tests to the executor API. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| """This function builds a ``CalibrationPlatform`` object which is sentitive of the hardware, | ||
| so it needs information about the clusters and its connection. Has to be used for acquisition. | ||
| """ |
| # but also we need the params saved in the datafolder, since in the | ||
| # platform folder might been changed. |
| self.platform | ||
| if ExecutionMode.ACQUIRE in mode | ||
| else CalibrationPlatform.from_datafolder( | ||
| folder_path=output, |
We noticed that when executing only FIT operation (e.g. though
qq fitcommand) Qibocal was building the platform using the state ofplatform_folder(e.g. the ones inqibolab_platforms_qrc) and not the state saved in theexperiment_data_folder.We fix this issue in the PR, also avoiding to construct the
Hardwaresensitive fields of thePlatformfor operations that do not include data acquisition.Also,
Completedobject now always requires apath:Pathinput an is no more optional, as I think it should be.Note
Discuss about eventual tests to add.
linked to Qibolab #1535.