✨ QDMI Environment Implementation#154
✨ QDMI Environment Implementation#154kayaercument wants to merge 76 commits intoMunich-Quantum-Software-Stack:developfrom
Conversation
|
Hey @kayaercument 👋🏼 (I think I can fix the coverage uploads from forks not working. let me check) |
mnfarooqi
left a comment
There was a problem hiding this comment.
@kayaercument
Here are a few minor comments while you are working on the tests.
…c and their tests
|
Dear @echavarria-lrz and @burgholzer, Please review the work. I will write the documentation after everything is finalised. |
ystade
left a comment
There was a problem hiding this comment.
@kayaercument thanks for the proposal of integrating the query of environmental data into QDMI. I took the liberty and added few questions inline in the code. Those are open questions more on the fundamental side. Hence, I think it makes sense to find an asnwer to them first, before I or someone else goes through the code in detail.
Overall, all additions fit niceliy with the look and feel of QDMI. Just one minor point: Already for a review of the code, doxygen comments help a lot to understand what the functions are supposed to do. However, since you did not actively request a review and I just voluntarily took a look, I cannot really complain.
|
Hey @burgholzer and @ystade, I and @mnfarooqi wanted to try |
Yeah. Something seems to be broken with CI at the moment. I'll check. |
|
CI is running again now. For some reason, the CI workflow was disabled 🤷🏼 |
It may be something to do with the settings in Codecov? For example: |
By now I tripple checked all settings across the GitHub repository, the GitHub organization as well as codecov. They perfectly match a known-good setup we use over at the MQT where codecov reports from forks work just fine without any special handling. |
ystade
left a comment
There was a problem hiding this comment.
Hi @kayaercument
@burgholzer asked me wether I can do another round of review as he couldn't find the time to do so. Hence, I started and got until the middle of the file include/qdmi/client.h (from the top in the "Files Changed" overview). Here in this file I realized that you do not seem to be done with the PR as a lot of my last comments lack a reaction. Consequently, I stopped in the middle and will wait for your ping when you wnt through the feedback. Thanks for the effort.
There was a problem hiding this comment.
@burgholzer What is the latest state here regarding the CodeCov setup? Should these changes be reverted?
There was a problem hiding this comment.
The current state is #154 (comment)
We should not need to explicitly set any secrets or tokens in workflows here. Any respective changes can be reverted here or should be moved to a separate PR that gets merged before this PR does.
There was a problem hiding this comment.
It is reverted. Have any support tickets been created?
There was a problem hiding this comment.
No. Would you mind creating one?
There was a problem hiding this comment.
Same holds for this file as commented above.
There was a problem hiding this comment.
It is reverted.
examples/device/device.cpp
Outdated
| param != QDMI_DEVICE_TELEMETRYSENSOR_QUERY_PARAMETER_CUSTOM3 && | ||
| param != QDMI_DEVICE_TELEMETRYSENSOR_QUERY_PARAMETER_CUSTOM4 && | ||
| param != QDMI_DEVICE_TELEMETRYSENSOR_QUERY_PARAMETER_CUSTOM5) || | ||
| value == nullptr || (size != sizeof(decltype(query->start_time)))) { |
There was a problem hiding this comment.
| value == nullptr || (size != sizeof(decltype(query->start_time)))) { | |
| value == nullptr) { |
The check (size != sizeof(decltype(query->start_time))) must be placed somewhere differently because also other information than of the start_time's type can be returned, e.g., a sensor pointer. This check should be handled in the respective cases.
| decltype(QDMI_device_session_query_telemetrysensor_property) | ||
| *device_session_query_telemetrysensor_property{}; | ||
|
|
||
| decltype(QDMI_device_telemetrysensor_query_set_parameter) | ||
| *device_telemetrysensor_query_set_parameter{}; | ||
|
|
||
| decltype(QDMI_device_session_create_device_telemetrysensor_query) | ||
| *device_session_create_device_telemetrysensor_query{}; | ||
|
|
||
| decltype(QDMI_device_telemetrysensor_query_submit) | ||
| *device_telemetrysensor_query_submit{}; | ||
|
|
||
| decltype(QDMI_device_telemetrysensor_query_get_results) | ||
| *device_telemetrysensor_query_get_results{}; | ||
|
|
||
| decltype(QDMI_device_telemetrysensor_query_check_status) | ||
| *device_telemetrysensor_query_check_status{}; | ||
|
|
||
| decltype(QDMI_device_telemetrysensor_query_wait) | ||
| *device_telemetrysensor_query_wait{}; | ||
|
|
||
| decltype(QDMI_device_telemetrysensor_query_cancel) | ||
| *device_telemetrysensor_query_cancel{}; | ||
|
|
||
| decltype(QDMI_device_telemetrysensor_query_free) | ||
| *device_telemetrysensor_query_free{}; | ||
|
|
There was a problem hiding this comment.
Would you mind adding docstrings similar to the lines above?
include/qdmi/client.h
Outdated
| * Additionally, the size of the buffer needed to retrieve the property is | ||
| * returned in @p size_ret if @p size_ret is not @c NULL. | ||
| * | ||
| * @note For example, to query the unit of an telemetry sensor, the following |
There was a problem hiding this comment.
| * @note For example, to query the unit of an telemetry sensor, the following | |
| * @note For example, to query the unit of a telemetry sensor, the following |
As mentioned above, please also check the rest.
include/qdmi/client.h
Outdated
| * returned in @p size_ret if @p size_ret is not @c NULL. | ||
| * | ||
| * @note For example, to query the unit of an telemetry sensor, the following | ||
| code pattern |
include/qdmi/client.h
Outdated
| * size_t telemetrysensor_unit_size = 0; | ||
| * auto ret = QDMI_device_query_telemetrysensor_property( | ||
| * device, telemetry_sensor, QDMI_TELEMETRYSENSOR_PROPERTY_UNIT, 0, | ||
| nullptr, |
There was a problem hiding this comment.
Please check for all other weird line breaks and fix them.
| /** @defgroup client_telemetrysensor_query_interface \ | ||
| * QDMI Client Telemetry Sensor Query Interface | ||
| * @brief Provides functions to query telemetry sensors. | ||
| * @details An telemetry sensor query is a task submitted by a client to a |
There was a problem hiding this comment.
| * @details An telemetry sensor query is a task submitted by a client to a | |
| * @details A telemetry sensor query is a task submitted by a client to a |
include/qdmi/client.h
Outdated
| * @see QDMI_Device_TelemetrySensor_Query for the device-side the telemetry | ||
| * sensor query handle. |
There was a problem hiding this comment.
Coud it be that there is one the too many?
Co-authored-by: Yannick Stade <100073938+ystade@users.noreply.github.com> Signed-off-by: Ercüment Kaya <49598189+kayaercument@users.noreply.github.com>
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (75.0%) is below the target coverage (90.0%). You can increase the patch coverage or adjust the target coverage. @@ Coverage Diff @@
## develop #154 +/- ##
=========================================
- Coverage 89.1% 85.9% -3.3%
=========================================
Files 6 6
Lines 712 920 +208
Branches 140 170 +30
=========================================
+ Hits 635 791 +156
- Misses 77 129 +52
🚀 New features to boost your workflow:
|
|
I am moving this to the 1.3.0 milestone for now as there still seems to be ongoing discussions that need to be resolved and the changes on |
Description
This PR bundles the required implementations to retrieve environmental telemetry data using QDMI.
Checklist
Closes: #152
Documentation will be created after details and naming are finalised.