Open alecandido opened 1 week ago
codecov[bot]
commented
1 week ago Attention: Patch coverage is 97.64398% with 9 lines in your changes missing coverage. Please review.
Project coverage is 97.18%. Comparing base (
4b8d407) to head (d2e8885). Report is 2 commits behind head on main.
alecandido
commented
1 week ago @andre-pasquale I should have made enough mess, now I will try to put the pieces back together.
However, I (more or less) reached the goal I had in mind, i.e. simplify the autocalibration command implementation, by delegating most of the operations to the new Output and friends.
https://github.com/qiboteam/qibocal/blob/c7b3da326c6a86c49947c4a43ac3ba2acb90b46f/src/qibocal/cli/autocalibration.py#L13-L52
The main reason is that for #869 most of the operations will be done by the executor. We can avoid dumping the runcard, since there won't be a runcard, and some operations might be left to the user. But I wanted to avoid leaving too many operations to the user, because it would just become boilerplate, to copy-paste in each and every script...
alecandido
commented
1 week ago By now, few operations are left:
Executor, so I'm planning to move it therethere are other two symmetric operations to be done before and after running
these I will delegate to the Executor connection and disconnection, that will perform these operations and handle the platform connection, respectively upon creation, and with an explicit call to Executor.close()
report() instead I will keep separate: it is a single function, so, if desired, it could be called at the end of the descript (otherwise just run qq report ... from the CLI later on)Eventually, the script should be simply something like:
from qibocal.routines import ..., close # <- here the default executor creation happens
# execute routines
...
close() # <- disconnect the platform and so onIt should be the minimal boilerplate I can leave to the user.
alecandido
commented
1 week ago The former means that the Executor will be in charge of everything, including dumping.
I wanted to isolate the disk operations from the execution, with two separate domains. But the trade-off is to handle two different objects, the Executor and the Output, with an explicit information flow from one to the other.
I'm not a huge fun of having everything happening under a single object, i.e. the Executor, so later on I might put some more effort to make it more modular.
However, for the time being that's a simple enough solution, and it will simplify the UX.
alecandido
commented
3 days ago I agree with all the minor changes, regarding some of the breaking changes that you highlighted I replied directly in the comments. Let me know when this is ready to review and I will have another look.
Unfortunately, I would have liked to postpone some changes, to immediately review & merge a smaller unit. That's why I split this before #869, despite that being the goal. I opened some issues for that purpose, but I'm not sure how many things I will be able to keep separate (e.g. I ended up implementing #911 here, at least preliminarily).
alecandido
commented
2 days ago @andrea-pasquale @Edoardo-Pedicillo I still need to finish providing tests, to improve coverage and test the new structures in isolation.
However, tests are already passing, and since this review might take a while I already switched to "ready for review". This is in order to accelerate the process, since this could be useful, and maybe enough to complete #869 as well.
Nevertheless, since I changed a few things, even in high-level behavior, I would ask you to run something to test that Qibocal is not broken in any major and fully undesirable way (not detected by tests).
Since I'm mostly playing with the output, and I almost didn't touch the routines (other than minimal support for lists), this should not require actual execution, but "dummy" should be more than enough.
alecandido
commented
2 days ago While testing qiboteam/qibolab_platforms_qrc#148 I got the following error which is not present in main. Most likely it is due to the fact that this particular platform is using strings (which I know you don't like as QubitId :) ). I don't know if we can force them to use
int, in any case if you could make the refactor works also with strings that would be perfect.
You're right I don't like, but I didn't anything specific to break it.
Can you tell me how to reproduce the error?
andrea-pasquale
commented
2 days ago While testing qiboteam/qibolab_platforms_qrc#148 I got the following error which is not present in main. Most likely it is due to the fact that this particular platform is using strings (which I know you don't like as QubitId :) ). I don't know if we can force them to use
int, in any case if you could make the refactor works also with strings that would be perfect.You're right I don't like, but I didn't anything specific to break it.
Can you tell me how to reproduce the error?
Sure, sorry for not specifying it before. I am running with this runcard
actions:
- id: qubit flux dependence
operation: qubit_flux
parameters:
bias_step: 0.004
bias_width: 0.2
drive_amplitude: 0.007
drive_duration: 1000
freq_step: 250000
freq_width: 20000000
nshots: 1000
relaxation_time: 10000
targets: null
update: true
backend: qibolab
platform: qw11q
targets:
- D1
- D2
- D3
- D4
- D5
update: trueOn the branch which I pointed out before in qibolab_platforms_qrc
alecandido
commented
1 day ago With QM I've not been able to reproduce the issue, because of the following error:
Traceback (most recent call last):
File "/home/users/alessandro.candido/.cache/pypoetry/virtualenvs/qrcli-S0iYXEb--py3.10/lib/python3.10/site-packages/qm/octave/calibration_db.py", line 98, in __del__
self._db.close()
AttributeError: 'CalibrationDB' object has no attribute '_db'(maybe I should fiddle a bit with the versions)
However, I already get an error using dummy, and there is a different error I obtained by not specifying the output folder (despite a default is available)
I will now focus on those. Then, I will ask you again for the environment. Mine is the following: https://github.com/alecandido/qrc/blob/ffc94221b866f6c5be538deac688a335c479b396/pyproject.toml#L17-L18 https://github.com/alecandido/qrc/blob/qibocal-909/.gitmodules
andrea-pasquale
commented
1 day ago With QM I've not been able to reproduce the issue, because of the following error:
Traceback (most recent call last): File "/home/users/alessandro.candido/.cache/pypoetry/virtualenvs/qrcli-S0iYXEb--py3.10/lib/python3.10/site-packages/qm/octave/calibration_db.py", line 98, in __del__ self._db.close() AttributeError: 'CalibrationDB' object has no attribute '_db'(maybe I should fiddle a bit with the versions)
However, I already get an error using
dummy, and there is a different error I obtained by not specifying the output folder (despite a default is available) The errorsI will now focus on those. Then, I will ask you again for the environment. Mine is the following: https://github.com/alecandido/qrc/blob/ffc94221b866f6c5be538deac688a335c479b396/pyproject.toml#L17-L18 https://github.com/alecandido/qrc/blob/qibocal-909/.gitmodules
Uhm, that is weird... Hopefully I didn't break anything when I pushed my changes in https://github.com/qiboteam/qibolab_platforms_qrc/pull/148 I'll try again
alecandido
commented
1 day ago Ah, apparently the two issues I described above are there even with main...
(switching to dummy)
And my bad: the error above was caused by me not exporting the environment variable QIBOLAB_PLATFORMS (on my machine I have it set by Nix, but there is no Nix on the QRC cluster, and I didn't study yet how to properly use it without admin privileges, so I'm definitely not proposing to use it there... yet...).
Now I've been able to run with https://github.com/qiboteam/qibocal/tree/main and qw11q.
alecandido
commented
1 day ago Ok, I tested again, and now I'm not getting any error at all...
I'm now using https://github.com/alecandido/qrc/commit/bda9c728a26d995d9d7cada38d50c6deac7ab267
Here is the command and the printed output:
alessandro.candido@saadiyat:~/qrc$ srun -p qw5q_platinum poetry run qq auto runcards/string-targets.yml -o var/ciao -f
2024-07-03 14:19:43,502 - qm - INFO - Starting session: 62b8d03d-b91d-4e14-bdb9-c2ee14973ce6
2024-07-03 14:19:46,470 - qm - INFO - Octave "octave4" Health check passed, current temperature 56
2024-07-03 14:19:47,389 - qm - INFO - Octave "octave5" Health check passed, current temperature 59
2024-07-03 14:19:48,405 - qm - INFO - Octave "octave6" Health check passed, current temperature 57
2024-07-03 14:19:48,405 - qm - INFO - Performing health check
2024-07-03 14:19:48,424 - qm - WARNING - Health check warning: Inter-OPX connectivity issues in OPX: con1. Missing ports are: 12, 11, 10, 9. See QM-App for more info.
2024-07-03 14:19:48,424 - qm - WARNING - Health check warning: Inter-OPX connectivity issues in OPX: con2. Missing ports are: 12, 11, 10, 9. See QM-App for more info.
2024-07-03 14:19:48,424 - qm - WARNING - Health check warning: Inter-OPX connectivity issues in OPX: con3. Missing ports are: 12, 11, 10, 9. See QM-App for more info.
2024-07-03 14:19:48,424 - qm - WARNING - Health check warning: Inter-OPX connectivity issues in OPX: con4. Missing ports are: 12, 11, 10, 9. See QM-App for more info.
2024-07-03 14:19:48,425 - qm - WARNING - Health check warning: Inter-OPX connectivity issues in OPX: con5. Missing ports are: 12, 11, 10, 9. See QM-App for more info.
2024-07-03 14:19:48,425 - qm - WARNING - Health check warning: Inter-OPX connectivity issues in OPX: con6. Missing ports are: 12, 11, 10, 9. See QM-App for more info.
2024-07-03 14:19:48,425 - qm - WARNING - Health check warning: Inter-OPX connectivity issues in OPX: con7. Missing ports are: 12, 11, 10, 9. See QM-App for more info.
2024-07-03 14:19:48,425 - qm - WARNING - Health check warning: Inter-OPX connectivity issues in OPX: con8. Missing ports are: 12, 11, 10, 9. See QM-App for more info.
2024-07-03 14:19:48,425 - qm - WARNING - Health check warning: Inter-OPX connectivity issues in OPX: con9. Missing ports are: 12, 11, 10, 9. See QM-App for more info.
2024-07-03 14:19:48,425 - qm - INFO - Health check passed
2024-07-03 14:19:50,190 - qm - INFO - Sending program to QOP for compilation
2024-07-03 14:19:50,681 - qm - WARNING - pulse 'drive(1000, 0.007, Gaussian(5))' used in play is not part of element 'driveD2' operations
2024-07-03 14:19:50,681 - qm - WARNING - pulse 'drive(1000, 0.007, Gaussian(5))' used in play is not part of element 'driveD3' operations
2024-07-03 14:19:50,682 - qm - WARNING - pulse 'drive(1000, 0.007, Gaussian(5))' used in play is not part of element 'driveD4' operations
2024-07-03 14:19:50,682 - qm - WARNING - pulse 'drive(1000, 0.007, Gaussian(5))' used in play is not part of element 'driveD5' operations
2024-07-03 14:19:50,682 - qm - WARNING - pulse 'readout(2000, 0.002, Rectangular())' used in measure is not part of element 'readoutD5' operations
2024-07-03 14:19:50,917 - qm - INFO - Executing programand here is a zip with all the output, including the report. test-qibocal-909.zip
Ok, the fits are weird, and I should check that's also the case with main, but I got no error...
alecandido
commented
1 day ago Ok, the fits are weird, and I should check that's also the case with
main, but I got no error...
Ok, apparently they're weird in the same way also with main. But it would be nice if you could reproduce it (either the behavior I'm observing or the error).
andrea-pasquale
commented
18 hours ago test-qibocal-909.zip
I think that zip is empty @alecandido
alecandido
commented
17 hours ago I think that zip is empty @alecandido
Correct, it was empty, my bad. I replaced it in the same comment.
alecandido
commented
17 hours ago Steps to reproduce:
git clone git@github.com:alecandido/qrc
cd qrc/
git switch qibocal-909
git submodule init
git submodule update
poetry install --with dev
export QIBOLAB_PLATFORMS=$(realpath ./qibolab_platforms_qrc)
srun -p qw5q_platinum poetry run qq auto runcards/string-targets.yml -o var/test-qibocal-909But this time I also failed with:
File "/nfs/users/alessandro.candido/qrc/qibocal/src/qibocal/auto/operation.py", line 136, in __getitem__
return self.data[qubit]
KeyError: ('D', '1')I can now investigate!
alecandido
commented
16 hours ago The problem is happening during the fit. And I know what is causing it:
https://github.com/qiboteam/qibocal/blob/7e57ee60575226a5a8b8049a9a72feb60e6ec23f/src/qibocal/auto/operation.py#L134-L135
i.e. while tuple and list are iterable, and int is not, a str is iterable as well.
In the process, I found out that, while the problem is occurring during the fit, and data have already been acquired, there are no data in the output folder, since the dump only happens at the end of Task.run().
This is very suboptimal, because in case of exceptions in the fit data are lost. I won't fix this immediately, to focus on closing this and #869.
But I will try to refactor the Task right after, tackling #908 and this is issue as well (which I'm about to open).
alecandido
commented
23 minutes ago @andrea-pasquale now it seems to work, though the fits are still failing...
test-qibocal-909.zip https://github.com/alecandido/qrc/commit/06a734b2498f6c678efad0f9ac583bd8875e0e13
The original goal was to make
Historythe manager of the whole output folder.It is almost already happening, but there are a few main exceptions:
meta.jsonruncard.yml(which will be optional, since the execution could be initiated by an arbitrary Python call)platform/andnew_platform/index.htmlTo avoid overcomplicating the
Historyobject, I will create a newOutputone, and make such that everything could be retrieved just with a handle on an instance of this object.The main motivation is to expose in a more structured way some operations that are now done internally by the CLI functions, e.g. https://github.com/qiboteam/qibocal/blob/4b8d407b11ebce72544821b361ea22df2f61c73a/src/qibocal/cli/fit.py#L58-L61 in such a way that it will be simpler to expose them for scripts, as those in #869