-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Executor context manager #949
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #949 +/- ##
==========================================
- Coverage 97.46% 97.46% -0.01%
==========================================
Files 120 120
Lines 9477 9494 +17
==========================================
+ Hits 9237 9253 +16
- Misses 240 241 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
bd6ee71
to
086951a
Compare
086951a
to
26e172f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @alecandido, LGTM.
I will test it shortly.
There is one thing still to be addressed after #942:
#942 (comment).
I think that we should expose also the update
attribute. If we don't it will be set to True
and all checks that we are performing in https://github.com/qiboteam/qibocal/blob/exec-ctx-manager/runcards/rx_calibration.py to assess when to update the platform don't make much sense. Let me know what you think.
Sorry, I've been too fast to merge that. But they are all additions, I will provide them here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @alecandido, works for me. Just a tiny comment.
Ok, tests are definitely not extensive, and they should be improved. However, the feature is there, and minimally tested. Since there are already other PRs relying on this, let's merge as soon as @Jacfomg confirms it is fine :) |
09ac57a
to
5683fe4
Compare
5683fe4
to
40153aa
Compare
To be more explicit as an example and template
40153aa
to
970776b
Compare
This is more or less complete as it is.
I will test, but it is already doing little to nothing.
Since this was originally an idea by @andrea-pasquale, I'd say it is ready for your review in the first place, and I'd like to collect your feedback.