[DEM-831] Compiler warnings in ZIHDAWG8 should be logged or raised. #1522

qSaevar · 2019-03-13T10:33:22Z

Changes proposed in this pull request:

Compiler warnings in ZIHDAWG8 are caught and logged
Compiler warnings can be elevated to errors

jenshnielsen · 2019-03-13T11:17:14Z

qcodes/instrument_drivers/ZI/ZIHDAWG8.py

+            for warning_as_error in self._warnings_as_errors:
+                if re.search(warning_as_error, warning) is not None:
+                    raise CompilerError('Warning treated as an error.', warning)
+            log.warning(warning)


I suggest using the log attached to the instrument see https://qcodes.github.io/Qcodes/examples/Creating%20Instrument%20Drivers.html#Logging for more info

codecov · 2019-03-13T12:57:56Z

Codecov Report

Merging #1522 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1522   +/-   ##
=======================================
  Coverage   73.84%   73.84%           
=======================================
  Files          92       92           
  Lines       10459    10459           
=======================================
  Hits         7723     7723           
  Misses       2736     2736

qSaevar · 2019-03-14T08:05:35Z

I addressed the comment and tested the branch on hardware. Is there anything else I should do?

astafan8

This in principle is OK-ish. Depending on how many people wanna use it.

I have two problems with this:

filtering with regexp is absolutely not convenient in this case, and requires some effort to test/debug. i wish these warnings had "id" by which one could decide to throw an exception with it or not.
if there are "errors", the only the first one is raised as an exception, and others are not included. I'd suggest that a more convenient way is to collect all "errors" and raise them as a single exception.

astafan8 · 2019-03-14T10:59:13Z

qcodes/instrument_drivers/ZI/ZIHDAWG8.py

-                      ['compiler']['sourcestring'][0]) > 0:
-            time.sleep(0.1)
+                  ['compiler']['sourcestring'][0]) > 0:
+            time.sleep(0.01)


please make at least a private attribute of the driver class out of this value.

astafan8 · 2019-03-14T11:02:06Z

qcodes/instrument_drivers/ZI/ZIHDAWG8.py

@@ -39,6 +53,19 @@ def __init__(self, name: str, device_id: str, **kwargs) -> None:
        self.awg_module.execute()
        node_tree = self.download_device_node_tree()
        self.create_parameters_from_node_tree(node_tree)
+        self._warnings_as_errors: List[str] = []
+
+    def elevate_compiler_warning_to_error(self, *warnings: str) -> None:


since there is this "add" functionality, i'd expect also a function for "removing" an already added warning so that it's treated again as a warning, right?)

to make it even easier, instead of creating the "add" and "remove" functions, i'd just make the _warnings_as_errors list "public" and write smth in the docs or the example notebook on how to use it.

qSaevar · 2019-03-14T11:36:44Z

filtering with regexp is absolutely not convenient in this case, and requires some effort to test/debug. i wish these warnings had "id" by which one could decide to throw an exception with it or not.

Filtering using regular expressions is needed as the status string has more info then just warnings. It is the output from the compiler and is only accessible as a single string. What type of "id" are you thinking about?

if there are "errors", the only the first one is raised as an exception, and others are not included. I'd suggest that a more convenient way is to collect all "errors" and raise them as a single exception.

That I will do.

qSaevar · 2019-03-14T12:21:40Z

@astafan8 I addressed the comments except the one with the error "ids".

astafan8

almost there :)

qcodes/instrument_drivers/ZI/ZIHDAWG8.py

Co-Authored-By: qSaevar <39056113+qSaevar@users.noreply.github.com>

astafan8 · 2019-03-14T14:56:57Z

qcodes/instrument_drivers/ZI/ZIHDAWG8.py

+        if len(errors) > 0:
+            raise CompilerError('Warning treated as an error.', *errors)
+
+        [self.log.warning(warning) for warning in warnings]


sorry, and now it would be great as a user to get the "warning" warnings before the "error" warnings so that you get the "full compiler response", not just the "error" part in case there are "error" warnings. Fair wish? :)

qcodes/instrument_drivers/ZI/ZIHDAWG8.py

qSaevar · 2019-03-14T15:27:37Z

I moved the logging of the warnings back into the for loop, but only log those that are not treated as errors.

astafan8

Brilliant! Thank you! Once the CI is done, i'll merge.

Merge: ebda0a8 6495a4a Author: Mikhail Astafev <astafan8@gmail.com> Merge pull request #1522 from qutech-sd/feature/DEM-831/Compiler-warnings-should-be-caught

jenshnielsen reviewed Mar 13, 2019

View reviewed changes

astafan8 approved these changes Mar 14, 2019

View reviewed changes

qSaevar added 3 commits March 14, 2019 12:31

[DEM-831] Compiler warnings in ZIHDAWG8 should be logged or raised.

15599d7

changed loggers and rebase on master

c95bd4b

remove unused import and added a typehint

1673852

Addressing code review comments

d35c1e8

astafan8 reviewed Mar 14, 2019

View reviewed changes

qcodes/instrument_drivers/ZI/ZIHDAWG8.py Outdated Show resolved Hide resolved

qcodes/instrument_drivers/ZI/ZIHDAWG8.py Outdated Show resolved Hide resolved

qcodes/instrument_drivers/ZI/ZIHDAWG8.py Outdated Show resolved Hide resolved

astafan8 and others added 2 commits March 14, 2019 15:13

Update qcodes/instrument_drivers/ZI/ZIHDAWG8.py

eb7b753

Co-Authored-By: qSaevar <39056113+qSaevar@users.noreply.github.com>

sleep and raise all warnings that should be raised

c6e3bba

astafan8 reviewed Mar 14, 2019

View reviewed changes

qcodes/instrument_drivers/ZI/ZIHDAWG8.py Outdated Show resolved Hide resolved

log warnings before errors

6495a4a

astafan8 approved these changes Mar 14, 2019

View reviewed changes

astafan8 merged commit 75fbea4 into microsoft:master Mar 14, 2019

qSaevar deleted the feature/DEM-831/Compiler-warnings-should-be-caught branch March 14, 2019 16:15

giulioungaretti pushed a commit that referenced this pull request Mar 14, 2019

Generated gh-pages for commit 75fbea4

7a92448

Merge: ebda0a8 6495a4a Author: Mikhail Astafev <astafan8@gmail.com> Merge pull request #1522 from qutech-sd/feature/DEM-831/Compiler-warnings-should-be-caught

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DEM-831] Compiler warnings in ZIHDAWG8 should be logged or raised. #1522

[DEM-831] Compiler warnings in ZIHDAWG8 should be logged or raised. #1522

qSaevar commented Mar 13, 2019

jenshnielsen Mar 13, 2019

qSaevar Mar 13, 2019

codecov bot commented Mar 13, 2019 •

edited

qSaevar commented Mar 14, 2019

astafan8 left a comment

astafan8 Mar 14, 2019

qSaevar Mar 14, 2019

astafan8 Mar 14, 2019

qSaevar Mar 14, 2019

qSaevar commented Mar 14, 2019

qSaevar commented Mar 14, 2019

astafan8 left a comment

astafan8 Mar 14, 2019

qSaevar Mar 14, 2019

qSaevar commented Mar 14, 2019

astafan8 left a comment

[DEM-831] Compiler warnings in ZIHDAWG8 should be logged or raised. #1522

[DEM-831] Compiler warnings in ZIHDAWG8 should be logged or raised. #1522

Conversation

qSaevar commented Mar 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 13, 2019 • edited

Codecov Report

qSaevar commented Mar 14, 2019

astafan8 left a 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

Choose a reason for hiding this comment

qSaevar commented Mar 14, 2019

qSaevar commented Mar 14, 2019

astafan8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qSaevar commented Mar 14, 2019

astafan8 left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 13, 2019 •

edited