Adding `qq update` command #905

andrea-pasquale · 2024-06-27T08:52:38Z

Addition of a qq update program to generate the updated platform from CLI after the qibocal execution. Closes #833.

Checklist:

Reviewers confirm new code works as expected.
Tests are passing.
Coverage does not decrease.
Documentation is updated.
Compatibility with Qibo modules (Please edit this section if the current pull request is not compatible with the following branches).
- Qibo: master
- Qibolab: main
- Qibolab_platforms_qrc: main

alecandido · 2024-06-27T09:12:32Z

src/qibocal/cli/update.py

+    runcard = Runcard.load(yaml.safe_load((path / RUNCARD).read_text()))
+    set_backend(backend=runcard.backend, platform=runcard.platform)
+    # run
+    history = run(
+        output=path,
+        runcard=runcard,
+        mode=ExecutionMode.UPDATE,
+    )


We should already have the most updated runcard in the output folder, when you invoke qq update.

Maybe it is in a subfolder, corresponding to the latest routine (which is perfectly fine), but I'd just maintain a link to the latest one in the top-level folder, and copy that one to the original platform.

In this way, you won't even need to track the timing. It's just a copy, it will take almost nothing.

We should already have the most updated runcard in the output folder, when you invoke qq update.

Maybe it is in a subfolder, corresponding to the latest routine (which is perfectly fine), but I'd just maintain a link to the latest one in the top-level folder, and copy that one to the original platform.

Actually this might not be the case. This is true only when running with AUTOCALIBRATION, but in this execution mode all updates will be performed in any case (local one after each protocol and the global one at the end).
I know that we still need to decide how to handle local updates. If by default we decide to keep local updates what you are suggesting makes sense. What do you think @alecandido?

alecandido · 2024-06-27T09:14:03Z

src/qibocal/update.py

    """Update drive frequency value in platform for specific qubit."""
-    if isinstance(freq, tuple):
+    if isinstance(freq, (tuple, list)):


What about just using one of the two?

This is just an ugly patch. I will try to improve it. The error was triggered when I tested with qq update, I think that there are some attributes in Results which are tuple since they contain also the error and when they are deserialized they are converted into lists.

Yes, JSON only supports one type of sequences, and that type is deserialized to list in Python.

However, if we want to serialize it, and we don't have advanced support to deserialize to our own types (e.g. Pydantic), I'd propose to not oppose to JSON, and just use lists. It's a practical solution to avoid an increased amount of code.

Implemented in cf006ca

I've been through cf006ca, and I marked a few tuples I've found, but there are many more.

Maybe I missed where the tuples had to be dropped from. Sorry...

alecandido · 2024-07-02T11:38:01Z

@andrea-pasquale I'm sorry for the tuple business. As soon as I will finish #909 I have other few simpler improvements in mind, including a consistent migration to Pydantic. Whenever that will be completed, tuples will be available again, since the serialization will come out of the box.

Unfortunately, I will have to balance those with #869 (but if I will be good enough with #909, a first version of that should be pretty simple to achieve).

andrea-pasquale · 2024-07-02T11:40:56Z

@andrea-pasquale I'm sorry for the tuple business. As soon as I will finish #909 I have other few simpler improvements in mind, including a consistent migration to Pydantic. Whenever that will be completed, tuples will be available again, since the serialization will come out of the box.

Unfortunately, I will have to balance those with #869 (but if I will be good enough with #909, a first version of that should be pretty simple to achieve).

No problem @alecandido.
Now I believe that it should be fixed. I will try to take care of tests now.
In which order do you recommend that we merge this alongside #909 and #869?

alecandido · 2024-07-02T13:40:14Z

In which order do you recommend that we merge this alongside #909 and #869?

Do not bother about #909 and #869: whenever you're ready, we review and merge. If it comes before the others, I will rebase myself ( ╥ ᴗ ╥)

codecov · 2024-07-02T13:44:49Z

Codecov Report

Attention: Patch coverage is 98.94737% with 1 line in your changes missing coverage. Please review.

Project coverage is 97.46%. Comparing base (4b8d407) to head (f0988b9).
Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #905      +/-   ##
==========================================
- Coverage   97.47%   97.46%   -0.02%     
==========================================
  Files         116      117       +1     
  Lines        8882     8906      +24     
==========================================
+ Hits         8658     8680      +22     
- Misses        224      226       +2

Flag	Coverage Δ
unittests	`97.46% <98.94%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
src/qibocal/auto/execute.py	`100.00% <100.00%> (ø)`
src/qibocal/auto/task.py	`99.15% <ø> (-0.04%)`	⬇️
src/qibocal/cli/_base.py	`91.93% <100.00%> (+0.70%)`	⬆️
src/qibocal/cli/autocalibration.py	`100.00% <100.00%> (ø)`
src/qibocal/cli/fit.py	`100.00% <100.00%> (ø)`
...rc/qibocal/protocols/coherence/spin_echo_signal.py	`100.00% <100.00%> (ø)`
src/qibocal/protocols/coherence/t1.py	`100.00% <100.00%> (ø)`
src/qibocal/protocols/coherence/t1_signal.py	`100.00% <100.00%> (ø)`
src/qibocal/protocols/coherence/t2_signal.py	`100.00% <100.00%> (ø)`
src/qibocal/protocols/coherence/utils.py	`95.38% <100.00%> (ø)`
... and 16 more

... and 1 file with indirect coverage changes

andrea-pasquale · 2024-07-02T15:48:59Z

Now it should be ready to review. There are a lot of file changes just for the tuple issue

alecandido

I have just a comment about local_update, but other than that it should be fine.

I'm also glad that the superposition with #909 is very small, it should be pretty simple to rebase :D

src/qibocal/cli/autocalibration.py

src/qibocal/cli/fit.py

src/qibocal/protocols/coherence/t1.py

Edoardo-Pedicillo · 2024-07-08T17:08:06Z

src/qibocal/auto/execute.py

            completed.update_platform(platform=self.platform, update=self.update)

+        if self.platform is not None:


Maybe you can move this line before if ExecutionMode.FIT in mode and self.platform is not None and update:

There is a small inconsistency with the typing, according to the Executor definition a platform is always defined even in the create method, for the moment could you change it ? (we can even wait #909)

You are right, the check now is needed only because we support still execution with simulation (where platform is None). Since #909 will drop it here we can keep it I think. I'll open an issue just to remember to do it.

andrea-pasquale added 3 commits June 27, 2024 11:34

feat: qq update first draft

26a4694

feat: Make update default using AUTOCALIBRATION

55b11c1

feat: Remove update from fit and fix some tests

fe86781

andrea-pasquale added the enhancement New feature or request label Jun 27, 2024

andrea-pasquale self-assigned this Jun 27, 2024

alecandido reviewed Jun 27, 2024

View reviewed changes

alecandido mentioned this pull request Jun 28, 2024

Generate updates #910

Open

andrea-pasquale added 3 commits July 2, 2024 10:32

fix: Drop tuple from results

cf006ca

fix: Fix lint

8f668bc

fix: Use list properly instead of tuples

a448002

andrea-pasquale added 3 commits July 2, 2024 17:23

feat: Proper implementation of qq update

c444819

fix: Minor fix in qq fit

32324d3

doc: Improve docstring

063b724

fix: Restore tests

7adab9f

test: Add qq update test

d60b2fc

andrea-pasquale requested review from alecandido and Edoardo-Pedicillo July 2, 2024 15:48

andrea-pasquale marked this pull request as ready for review July 2, 2024 18:43

andrea-pasquale changed the title ~~[poc] Adding qq update command~~ Adding qq update command Jul 2, 2024

doc: Add qq update doc

9da5af8

alecandido reviewed Jul 3, 2024

View reviewed changes

src/qibocal/cli/autocalibration.py Outdated Show resolved Hide resolved

src/qibocal/cli/fit.py Outdated Show resolved Hide resolved

refactor: Dump platform always

9bef476

alecandido approved these changes Jul 5, 2024

View reviewed changes

fix: Fix platform None in tests

239289c

feat: Add info when running qq update

cb88306

andrea-pasquale mentioned this pull request Jul 8, 2024

Move output folder management #909

Merged

Edoardo-Pedicillo reviewed Jul 8, 2024

View reviewed changes

src/qibocal/protocols/coherence/t1.py Outdated Show resolved Hide resolved

fix: Fix type hint

f0988b9

Edoardo-Pedicillo reviewed Jul 8, 2024

View reviewed changes

Edoardo-Pedicillo approved these changes Jul 8, 2024

View reviewed changes

andrea-pasquale mentioned this pull request Jul 8, 2024

Drop check platform None when updating platform #925

Closed

andrea-pasquale added this pull request to the merge queue Jul 8, 2024

Merged via the queue into main with commit c3f355f Jul 8, 2024
21 checks passed

andrea-pasquale deleted the qq-update branch July 8, 2024 17:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding `qq update` command #905

Adding `qq update` command #905

andrea-pasquale commented Jun 27, 2024 •

edited

Loading

alecandido Jun 27, 2024

alecandido Jun 27, 2024

andrea-pasquale Jul 2, 2024

alecandido Jun 27, 2024

andrea-pasquale Jun 27, 2024

alecandido Jun 27, 2024 •

edited

Loading

andrea-pasquale Jul 2, 2024

alecandido Jul 2, 2024

alecandido commented Jul 2, 2024

andrea-pasquale commented Jul 2, 2024

alecandido commented Jul 2, 2024

codecov bot commented Jul 2, 2024 •

edited

Loading

andrea-pasquale commented Jul 2, 2024

alecandido left a comment

Edoardo-Pedicillo Jul 8, 2024 •

edited

Loading

andrea-pasquale Jul 8, 2024

		completed.update_platform(platform=self.platform, update=self.update)

		if self.platform is not None:

Adding qq update command #905

Adding qq update command #905

Conversation

andrea-pasquale commented Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecandido Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecandido commented Jul 2, 2024

andrea-pasquale commented Jul 2, 2024

alecandido commented Jul 2, 2024

codecov bot commented Jul 2, 2024 • edited Loading

Codecov Report

andrea-pasquale commented Jul 2, 2024

alecandido left a comment

Choose a reason for hiding this comment

Edoardo-Pedicillo Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adding `qq update` command #905

Adding `qq update` command #905

andrea-pasquale commented Jun 27, 2024 •

edited

Loading

alecandido Jun 27, 2024 •

edited

Loading

codecov bot commented Jul 2, 2024 •

edited

Loading

Edoardo-Pedicillo Jul 8, 2024 •

edited

Loading